feat: implement landing page and sidebar UI#22
Conversation
Add public landing page and PenguinUI sidebar navigation. Still needs browser cache clearing and layout fixes. Partial implementation of #19
📝 WalkthroughWalkthroughAdds public site/layout and landing pages, new App Version DocType, CI runtime updates, UI and layout refactors (sidebar, mask plugin, theme tokens), Custom Image and Apps privacy/builds refactor with new APIs for registering/deleting custom apps, Ansible clone replaced with fresh checkout, and i18n of tooling errors. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Browser (User)
participant Modal as Custom App Modal (UI)
participant Client as Client Store/Validator
participant Backend as Server API
participant Apps as Apps DocType (DB)
User->>Modal: Open modal, choose Public/Private, fill name, githubUrl, branch, token?
Modal->>Client: Validate inputs
alt Valid
Client->>Backend: register_custom_app(name, githubUrl, branch, token?, order?)
Backend->>Apps: create or update Apps record (is_custom=1)
Apps-->>Backend: saved
Backend-->>Client: success {app_reference_id, message}
Client->>Modal: reset form, close modal
Modal->>User: show success toast
else Invalid
Client-->>Modal: return validation errors
Modal->>User: show errors
end
sequenceDiagram
participant User as Browser (User)
participant DomainComp as Domain Component (UI)
participant Verifier as Client Verifier
participant DNS as DNS/Resolver
participant Store as Local Store
User->>DomainComp: Enter domain, click Next
DomainComp->>Store: clear domainVerified
DomainComp->>Verifier: trigger verification (always if domain provided)
Verifier->>DNS: resolve / check A record
alt DNS verifies
DNS-->>Verifier: verified
Verifier->>Store: set domainVerified = true
Verifier->>DomainComp: advance to next step
else Not verified
DNS-->>Verifier: not found/error
Verifier->>Store: set domainVerified = false
Verifier->>DomainComp: show error toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
Update the sidebar to include the profile in the bottom and also increased the width of the center container |
- Fix success toast notification color from red to green - Replace text "Remove" button with Heroicons trash icon in red
…he app is public or private, based on this toggle if the github token is required or not.
|
@Venkateshvenki404224 the linters are failing |
I am on it |
- Split apps into two distinct sections with headers - Frappe Apps section displays core apps (frappe === 1) with green gradient selection - Custom Apps section shows database custom apps and user-added apps - Updated backend to fetch 'frappe' field from Apps doctype - Added computed properties to filter apps by type - Implemented inline error validation for custom app form - Fixed domain verification to always re-verify on each visit
Venkateshvenki404224
force-pushed
the
ui-changes
branch
from
ddbe7c0 to
d46f5dc
Compare
| # scheduler_events = { | ||
| # "cron": { | ||
| # "*/5 * * * *": [ | ||
| # "nano_press.utils.ansible_runner.ping_server", | ||
| # ], | ||
| # } | ||
| # } |
There was a problem hiding this comment.
Why are we removing this?
There was a problem hiding this comment.
Because the ping server function expects ip address or server doc so I didn't have any idea on how to pass argument from hooks.py
nano_press/public/js/script.js
Outdated
| }); | ||
| this.customAppForm = { name: '', githubUrl: '', token: '' }; | ||
| this.showCustomAppDialog = false; | ||
| // Clear all errors |
There was a problem hiding this comment.
Is this comment needed?
nano_press/public/js/script.js
Outdated
| this.formErrors = { name: '', githubUrl: '', token: '', branch: '' }; | ||
|
|
||
| let hasError = false; | ||
|
|
||
| // Validate all required fields | ||
| if (!this.customAppForm.name) { | ||
| this.formErrors.name = 'App name is required.'; | ||
| hasError = true; | ||
| } | ||
|
|
||
| if (!this.customAppForm.githubUrl) { | ||
| this.formErrors.githubUrl = 'GitHub URL is required.'; | ||
| hasError = true; | ||
| } else if (!this.customAppForm.githubUrl.startsWith('https://')) { | ||
| this.formErrors.githubUrl = 'GitHub URL must start with https://'; | ||
| hasError = true; | ||
| } | ||
|
|
||
| if (this.appType === 'private' && !this.customAppForm.token) { | ||
| this.formErrors.token = | ||
| 'GitHub Token is required for private repositories.'; | ||
| hasError = true; | ||
| } | ||
|
|
||
| if (!this.customAppForm.branch) { | ||
| this.formErrors.branch = 'Branch is required.'; | ||
| hasError = true; |
There was a problem hiding this comment.
Instead of manually checking can we use the HTML validation like this https://github.com/BuildWithHussain/ls_shop/blob/develop/ls_shop/www/account/profile.html
On form submit we collect the data and add additional validations, everything else is handled by HTML form validations
| <!-- SECTION 2: Custom Apps --> | ||
| <h3 class="text-sm font-semibold mb-3 mt-6 text-gray-900">Custom Apps</h3> | ||
|
|
||
| <!-- Database custom apps (frappe === 0) --> |
There was a problem hiding this comment.
Can we remove this comment?
| </div> | ||
| </template> | ||
|
|
||
| <!-- "Add Custom App" Button --> |
There was a problem hiding this comment.
Let's remove this and other comments
| {% set full_name = frappe.utils.get_fullname(frappe.session.user) if frappe.utils.get_fullname else frappe.session.user %} | ||
|
|
||
| <div x-data="{ showSidebar: false }" class="relative flex w-full flex-col md:flex-row"> | ||
| <!-- This allows screen readers to skip the sidebar and go directly to the main content. --> |
There was a problem hiding this comment.
Let's remove this comments
| {% include "templates/includes/sidebar.html" %} | ||
|
|
||
| <div class="overflow-y-auto" x-data="toaster"> | ||
| <div class="w-full px-6 md:px-10 py-8"> |
There was a problem hiding this comment.
Let's cap the width at max-w-7xl.
Reason being on extremely large devices it takes complete width
It's better to have it contained.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nano_press/nano_press/doctype/custom_image/custom_image.py (1)
90-97: PAT token is exposed via API endpoint and should not be included in responses sent to the frontend.The
_build_repo_url()function correctly embeds the PAT token into the URL for private repository access. However, on line 54, this entire apps configuration (including the token-embedded URL) is serialized to JSON viajson.dumps(sorted_apps, indent=2). This JSON is then returned by the@frappe.whitelist()methodpreview_apps_json_for_form()(lines 142-149), exposing the PAT token to the frontend and any API client. The token should be stripped from responses sent to clients—only the server should retain it when executing git clone operations.nano_press/templates/components/core/summary.html (1)
452-479: Add timeout to prevent indefinite polling.
pollForBuildCompletionpolls indefinitely with no maximum attempts or timeout. If the build never completes or fails silently, this will poll forever, consuming resources and leaving the user stuck.🔎 Proposed fix with timeout
async pollForBuildCompletion(customImageName) { + const maxAttempts = 120; // 10 minutes at 5-second intervals + let attempts = 0; return new Promise((resolve, reject) => { const pollInterval = setInterval(async () => { try { + attempts++; + if (attempts >= maxAttempts) { + clearInterval(pollInterval); + reject(new Error('Build timed out after 10 minutes')); + return; + } + const url = '/api/method/nano_press.nano_press.doctype.custom_image.custom_image.get_build_status'; const payload = { custom_image_name: customImageName }; const response = await frappe_call(url, payload, "POST"); if (response?.message) { const status = response.message.status; if (status === 'Built') { clearInterval(pollInterval); resolve(customImageName); } else if (status === 'Failed') { clearInterval(pollInterval); reject(new Error('Custom image build failed')); } } } catch (error) { console.error('Failed to poll build status:', error); + // Stop polling after multiple consecutive failures + if (attempts >= 3) { + clearInterval(pollInterval); + reject(new Error('Failed to check build status')); + } } }, 5000); }); },
🧹 Nitpick comments (10)
.github/workflows/ci.yml (1)
54-54: Update actions/setup-node to v4.The
actions/setup-node@v3action is deprecated. Please update to v4 for continued support.As per static analysis hints, the runner for
actions/setup-node@v3is too old.🔎 Proposed fix
- - name: Setup Node - uses: actions/setup-node@v3 + - name: Setup Node + uses: actions/setup-node@v4nano_press/www/index.py (1)
1-6: Consider clarifying the dual no_cache settings.Both the module-level
no_cache = 1andcontext.no_cache = 1insideget_context()serve the same purpose. If both are required by the framework, a brief comment explaining why would help future maintainers. Otherwise, one may be redundant.🔎 Proposed clarification
+# Module-level no_cache prevents page caching at the route level no_cache = 1 def get_context(context): + # Context-level no_cache ensures dynamic rendering on each request context.no_cache = 1 return contextnano_press/templates/components/landing/features.html (1)
82-82: Minor: Inconsistent spacing in "Real time Logs".Consider using "Real-time Logs" (hyphenated) for consistency with standard English usage.
nano_press/templates/includes/sidebar.html (1)
61-67: Consider POST for logout action.Using a GET request (
href) for logout is functional but not ideal since it's a state-changing action. Modern best practices recommend POST requests for logout to prevent CSRF and accidental logouts via prefetching. However, this follows Frappe's standard pattern.nano_press/templates/layout.html (1)
56-71: Document the split-tag pattern between sidebar.html and layout.html.The closing
</div>tags on lines 69-71 close elements opened insidebar.html(which opens 10 divs but closes only 7). This split-tag pattern across files is fragile—changes to either file's div structure can break the layout. Add inline comments documenting this dependency in both files.nano_press/templates/components/core/domain.html (1)
23-30: Consider hiding DNS instructions when server IP is unavailable.The fallback
'0.0.0.0'could mislead users into creating an A record pointing to an invalid IP. Consider hiding the DNS instructions entirely until a valid server IP is available, or displaying a more explicit placeholder like "Server IP not configured".🔎 Proposed enhancement
- <div x-show="$store.installer.domain && $store.installer.domain.trim()" class="mt-3"> + <div x-show="$store.installer.domain && $store.installer.domain.trim() && $store.installer.serverDetails?.ip" class="mt-3"> <div class="bg-blue-50 border border-blue-200 rounded-md p-4"> <p class="text-sm text-gray-700 mb-2">Point your domain to the server by creating an <span class="font-semibold">A record</span>:</p> <p class="font-mono text-sm text-gray-900 font-medium"> - <span x-text="$store.installer.domain"></span> → <span x-text="$store.installer.serverDetails?.ip || '0.0.0.0'"></span> + <span x-text="$store.installer.domain"></span> → <span x-text="$store.installer.serverDetails?.ip"></span> </p> </div> </div> + <div x-show="$store.installer.domain && $store.installer.domain.trim() && !$store.installer.serverDetails?.ip" class="mt-3"> + <div class="bg-yellow-50 border border-yellow-200 rounded-md p-4"> + <p class="text-sm text-yellow-700">Please verify your server first to see DNS instructions.</p> + </div> + </div>nano_press/templates/components/core/summary.html (1)
128-141: Consider adding a password visibility toggle.The admin password is displayed in plain text, which could be a security concern during screen-sharing or if someone is looking over the user's shoulder. Consider adding a show/hide toggle for the password field.
nano_press/public/js/script.js (1)
71-120: Consider preventing duplicate app names.The
addCustomApp()function doesn't check if an app with the same name already exists incustomApps. This could lead to duplicate entries that might cause issues during deployment.🔎 Proposed enhancement
addCustomApp() { // Clear all errors this.formErrors = { name: '', githubUrl: '', token: '', branch: '' }; let hasError = false; // Validate all required fields if (!this.customAppForm.name) { this.formErrors.name = 'App name is required.'; hasError = true; + } else if (this.customApps.some(app => app.name.toLowerCase() === this.customAppForm.name.toLowerCase())) { + this.formErrors.name = 'An app with this name already exists.'; + hasError = true; }nano_press/templates/components/core/apps.html (2)
231-277: Duplicate validation logic withscript.js.The
addCustomApp()method here duplicates the validation logic innano_press/public/js/script.js(lines 71-120). This creates maintenance burden and risk of divergence. Consider consolidating the validation logic in one place and referencing it from both components.
206-218: Duplicate state definitions withscript.js.
formErrors,appType, andlocalCustomAppFormstructures are duplicated here and inscript.js. While component-local state is valid, consider whether the installer store (window.frappeInstaller) could serve as the single source of truth for these form states, with components accessing them via$store.installer.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
nano_press/public/images/nano-press.pngis excluded by!**/*.png
📒 Files selected for processing (23)
.github/workflows/ci.ymlnano_press/hooks.pynano_press/nano_press/doctype/apps/apps.jsonnano_press/nano_press/doctype/apps/apps.pynano_press/nano_press/doctype/custom_image/custom_image.pynano_press/nano_press/utils/ansible/playbooks/build_custom_image.ymlnano_press/public/css/theme.cssnano_press/public/js/script.jsnano_press/public/js/vendor/alpinejs/plugins/mask.jsnano_press/templates/components/core/apps.htmlnano_press/templates/components/core/domain.htmlnano_press/templates/components/core/stepper.htmlnano_press/templates/components/core/summary.htmlnano_press/templates/components/landing/features.htmlnano_press/templates/components/landing/hero.htmlnano_press/templates/includes/home/custom_app_modal.htmlnano_press/templates/includes/sidebar.htmlnano_press/templates/includes/toast-teleport.htmlnano_press/templates/layout.htmlnano_press/templates/layout_public.htmlnano_press/www/index.htmlnano_press/www/index.pynano_press/www/self_host.py
🧰 Additional context used
🧬 Code graph analysis (2)
nano_press/public/js/vendor/alpinejs/plugins/mask.js (1)
nano_press/public/js/vendor/alpinejs/core.js (1)
i(5-5)
nano_press/nano_press/doctype/apps/apps.py (2)
nano_press/nano_press/doctype/frappe_site/frappe_site.py (2)
before_insert(43-44)validate(62-65)nano_press/nano_press/doctype/server/server.py (1)
validate(43-48)
🪛 actionlint (1.7.9)
.github/workflows/ci.yml
54-54: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (30)
.github/workflows/ci.yml (2)
48-51: LGTM: Python 3.14 upgrade is valid.Python 3.14 was released on October 7, 2025, with the latest maintenance release being 3.14.2 from December 5, 2025. This upgrade is appropriate and brings improvements including free-threaded Python support, deferred annotation evaluation, and template string literals.
53-57: LGTM: Node.js 24 upgrade is valid.Node.js 24 was released in May 2025 with V8 13.6 and npm 11, and entered Long Term Support (LTS) in October 2025 with support continuing through April 2028. This upgrade brings performance improvements, AsyncContextFrame for AsyncLocalStorage, and improved HTTP capabilities via Undici 7.
nano_press/templates/components/core/stepper.html (1)
8-8: LGTM – z-index adjustment addresses layering.The reduction from z-index 10 to 0 aligns with the PR objective to fix stepper overlap issues with the navbar.
nano_press/templates/includes/toast-teleport.html (1)
343-343: Critical UX fix: success toasts now display green.This corrects a critical visual bug where success messages were incorrectly styled with red text (text-red-900), which typically indicates errors or warnings. The new green color (text-green-600) provides appropriate positive feedback to users.
nano_press/nano_press/utils/ansible/playbooks/build_custom_image.yml (1)
32-42: Improved reliability with fresh clone approach.The two-step flow (remove directory, then fresh clone) ensures a pristine checkout and eliminates risks of stale files or partial state from previous runs. This is more robust than the prior update-with-force approach, though it trades some speed for reliability.
nano_press/hooks.py (2)
71-71: LGTM – home page now points to the new landing page.Changing from "login" to "index" aligns with the PR's introduction of a public landing page at
nano_press/www/index.html.
154-160: Consider deleting or documenting the disabled scheduler.The commented
scheduler_eventscontainingping_serverwill no longer execute automatic health checks every 5 minutes. Theping_serverfunctions remain innano_press/utils/ansible_runner.pyandnano_press/api.pyand are still accessible via the API, so they can be called manually if needed. However, keeping commented code can cause confusion. Either delete it permanently or add a TODO comment explaining why it's disabled and when it should be re-enabled.nano_press/www/self_host.py (1)
13-18: LGTM – additional field supports enhanced app management.Including the
frappefield in the query aligns with the PR's broader app management enhancements, providing additional metadata for distinguishing Frappe apps from custom apps.nano_press/www/index.html (2)
1-36: Landing page template looks good.The template is well-structured with:
- Proper Jinja2 inheritance from the public layout
- Semantic HTML sections
- Accessible CTAs with descriptive text and icons
- Clean Tailwind CSS styling
24-24: No action needed on hardcoded GitHub URL.The URL is correct and consistent across all three occurrences in the codebase (index.html, hero.html, footer.html). Hardcoding a project's own GitHub repository link is standard practice and doesn't require configuration via environment variables or site settings.
Likely an incorrect or invalid review comment.
nano_press/nano_press/doctype/apps/apps.py (2)
9-29: Auto-generated type hints placement is unconventional but functional.The TYPE_CHECKING block is placed inside the class body, which is unusual but works in Python. This pattern is likely generated by Frappe's tooling for DocType type hints.
34-39: Incomplete handling whenis_customis False.The
validate()method only setsis_publicwhenis_customis truthy. Ifis_customis False (i.e., a Frappe app),is_publicremains unchanged. Verify this is intentional—if Frappe apps should always be public, consider explicitly settingis_public = 1for that case.🔎 Suggested fix if Frappe apps should always be public
def validate(self): if self.is_custom: if self.pat_token: self.is_public = 0 else: self.is_public = 1 + else: + # Non-custom (Frappe) apps are always public + self.is_public = 1nano_press/templates/includes/home/custom_app_modal.html (2)
7-27: LGTM! Good accessibility implementation.The tabbed UI correctly implements ARIA roles (
tablist,tab) andaria-selectedstates for screen reader support. The conditional styling provides clear visual feedback.
46-52: Verify token field visibility matches validation logic.The GitHub Token field is shown when
appType === 'private'. Ensure the correspondingaddCustomApp()validation inscript.jsrequires the token only whenappType === 'private'to avoid validation mismatches.nano_press/templates/components/landing/hero.html (1)
35-56: Nice terminal demo for the landing page!The simulated terminal output provides a clear visual of the deployment experience. The animated cursor adds a polished touch.
nano_press/templates/layout.html (1)
24-24: LGTM - mask.js plugin added.The Alpine.js mask plugin is correctly included after the other plugins and before the core script.
nano_press/templates/includes/sidebar.html (2)
4-6: Good accessibility: skip-to-content link implemented.The skip link allows keyboard and screen reader users to bypass navigation and jump directly to main content.
44-71: Good keyboard navigation support.The profile menu properly implements keyboard navigation with
x-on:keydown.down.preventandx-on:keydown.up.preventfor arrow key navigation, plusx-trapfor focus management.nano_press/nano_press/doctype/custom_image/custom_image.py (2)
88-103: Good defensive handling for missing repo_url.The guard against missing
repo_urlwith a default empty string prevents AttributeError. The updated logic usingnot is_publicaligns with the new data model.
369-382: LGTM - Apps payload updated correctly.The field renames (
app_name,pat_token) and new fields (enabled,frappe) align with the Apps DocType schema changes.nano_press/public/js/vendor/alpinejs/plugins/mask.js (1)
1-1: Vendor code - no review required.This is a minified third-party Alpine.js mask plugin. Ensure it's sourced from the official Alpine.js plugins repository or a trusted source for security.
nano_press/templates/components/core/domain.html (2)
69-74: Condition inupdateDomainmay not work as intended.The check
value !== this.$store.installer.domaincompares the new value against the current store value before updating it. This means if the user types, the condition will almost always be true (sincevalueis the new input andthis.$store.installer.domainis the old value). ThedomainVerifiedflag should be reset whenever the domain changes from its verified state, but the current condition checksdomainVerified && value !== ...which is correct logic.However, note that line 73 updates
this.$store.installer.domain = valueAFTER the check, so the comparison is actually correct (comparing new vs old). No change needed here.
80-118: LGTM!The
nextStep()logic correctly enforces domain verification every time the user proceeds, which is a good defensive approach. The error handling and state management are well-structured.nano_press/templates/components/core/summary.html (1)
86-107: LGTM!The step indicator logic correctly shows yellow for the current step and green for completed steps. The conditional classes are well-structured and consistent across all deployment phases.
nano_press/public/js/script.js (1)
26-38: LGTM!The form state structure with
formErrorsandcustomAppForm(including the newbranchfield) provides good support for inline validation and error messaging.nano_press/nano_press/doctype/apps/apps.json (2)
55-61: LGTM!Good changes: changing
pat_tokentoPasswordfieldtype improves security by masking the value, and the updateddepends_oncorrectly ties the token visibility to custom apps (is_custom == 1).
92-101: LGTM!The new
is_customfield provides a clear distinction between Frappe apps and custom apps, enabling the filtering logic in the UI.nano_press/templates/components/core/apps.html (3)
26-39: LGTM!The Frappe Apps grid with toggle functionality and conditional styling for selected state is well-implemented. The case-insensitive comparison using
toLowerCase()is applied consistently.
100-121: Good accessibility implementation.The tab implementation properly uses ARIA roles (
role="tablist",role="tab") andaria-selectedattributes for the Public/Private app type selection. This enhances keyboard navigation and screen reader support.
192-199: LGTM!The computed getters
frappeAppsandcustomAppsprovide clean filtering based on thefrappefield. This separates apps into their respective sections in the UI.
| /* Penguin UI Design Tokens */ | ||
| --color-primary: oklch(45.56% 0.237 264.05); | ||
| --color-primary-dark: oklch(70% 0.2 264.05); | ||
|
|
||
| /* Surface colors */ | ||
| --color-surface: oklch(100% 0 0); | ||
| --color-surface-alt: oklch(98% 0 0); | ||
| --color-surface-dark: oklch(23% 0.011 285.82); | ||
| --color-surface-dark-alt: oklch(20% 0.011 285.82); | ||
|
|
||
| /* Text colors */ | ||
| --color-on-surface: oklch(40% 0.019 285.88); | ||
| --color-on-surface-strong: oklch(20% 0.019 285.88); | ||
| --color-on-surface-dark: oklch(90% 0.005 285.82); | ||
| --color-on-surface-dark-strong: oklch(100% 0 0); | ||
| --color-on-primary: oklch(100% 0 0); | ||
| --color-on-primary-dark: oklch(20% 0.019 285.88); | ||
|
|
||
| /* Border colors */ | ||
| --color-outline: oklch(85% 0.005 285.82); | ||
| --color-outline-dark: oklch(30% 0.011 285.82); | ||
|
|
||
| /* Border radius */ | ||
| --radius: 0.5rem; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
oklch CSS color format browser support compatibility
💡 Result:
Short summary
- OKLCH (oklch()) is widely supported in modern browsers but not universal; most current desktop/mobile releases of Chrome, Edge and Firefox, and recent Safari versions support it, while older browsers and some WebView/Samsung-Internet builds may not. [1][2][3]
Key support details
- Can I Use shows broad support across modern Chrome, Edge and Firefox releases and Safari (check specific versions for your audience). It also lists gaps for older releases, some Android/WebView variants, and a few niche browsers. [2]
- MDN documents the oklch() syntax and compatibility notes. [1]
- Guides recommend providing fallbacks because some users run older browsers or un-updated devices (e.g., older Samsung Internet or legacy Android). [3]
Practical fallback pattern (use before the OKLCH value)
- Provide an sRGB/HEX/RGB fallback, then override inside an @supports block that checks OKLCH support. Example:
background: #6495ed; /* fallback */
@supports (color: oklch(0% 0 0)) {
background: oklch(70% 0.15 240);
}
(Adapt values as needed.) [3]
If you want, I can:
- look up exact minimum browser versions for a specific audience (desktop/mobile %, or by country) and give a precise compatibility table, or
- generate PostCSS/Build-tool fallbacks or polyfill recommendations.
Add fallback values for oklch() colors.
The oklch() format lacks universal browser support—older browsers and some Android/WebView variants do not support it. Provide fallback values using standard CSS colors (hex, RGB) before each oklch declaration, or wrap oklch declarations in an @supports rule that checks for color format support.
Example pattern:
--color-primary: #6495ed; /* fallback */
@supports (color: oklch(0% 0 0)) {
--color-primary: oklch(45.56% 0.237 264.05);
}This ensures older browsers can still render the design using fallback colors.
🤖 Prompt for AI Agents
nano_press/public/css/theme.css lines 12-36: the CSS currently uses only oklch()
color functions which lack universal browser support; add fallback color values
(hex or rgb) before each oklch() variable or wrap the oklch() assignments in
@supports(color: oklch(...)) blocks so browsers that don’t understand oklch will
use the fallback. For each variable, insert a conservative hex/rgb fallback on
the same property first (e.g. --color-primary: #xxxxxx;) then override with the
oklch() inside an @supports rule (or simply place the fallback before the
oklch() definition) and repeat for all oklch() variables (primary, surface,
text, outline, etc.), choosing visually close fallback values and keeping the
same variable names and ordering so existing usage is unchanged.
| </div> | ||
|
|
||
| <div class="grid grid-cols-1 md:grid-cols-3 gap-8"> | ||
| <div class="group p-8 from-gray-50 to-white border border-gray-200 rounded-xl hover:shadow-xl transition-all duration-300 hover:-translate-y-1"> |
There was a problem hiding this comment.
Missing gradient direction class.
The class from-gray-50 to-white won't produce a gradient without a bg-gradient-to-* direction class (e.g., bg-gradient-to-b). This also applies to lines 25 and 37.
🔎 Proposed fix
-<div class="group p-8 from-gray-50 to-white border border-gray-200 rounded-xl hover:shadow-xl transition-all duration-300 hover:-translate-y-1">
+<div class="group p-8 bg-gradient-to-b from-gray-50 to-white border border-gray-200 rounded-xl hover:shadow-xl transition-all duration-300 hover:-translate-y-1">Apply similarly to lines 25 and 37.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="group p-8 from-gray-50 to-white border border-gray-200 rounded-xl hover:shadow-xl transition-all duration-300 hover:-translate-y-1"> | |
| <div class="group p-8 bg-gradient-to-b from-gray-50 to-white border border-gray-200 rounded-xl hover:shadow-xl transition-all duration-300 hover:-translate-y-1"> |
🤖 Prompt for AI Agents
In nano_press/templates/components/landing/features.html around lines 13, 25 and
37 the utility classes use "from-gray-50 to-white" but are missing a gradient
direction class so no gradient will render; add an appropriate Tailwind gradient
direction class (for example "bg-gradient-to-b" or "bg-gradient-to-r" as design
requires) to each of those divs so the from/to colors produce a visible
gradient.
There was a problem hiding this comment.
We don't need gradient
There was a problem hiding this comment.
@Rl0007, understood! I'll skip the gradient suggestion. If those from-* and to-* classes aren't needed, feel free to remove them to keep the markup clean, but that's entirely up to you. 😊
| <a href="https://github.com/BuildWithHussain/nano_press" | ||
| target="_blank" | ||
| class="inline-flex items-center justify-center px-8 py-4 text-lg font-semibold text-gray-700 bg-white border-2 border-gray-300 rounded-lg hover:border-gray-400 transition-all duration-200 shadow-md hover:shadow-lg"> | ||
| <svg xmlns="http://www.w3.org/2000/svg" class="w-5 h-5 mr-2" fill="currentColor" viewBox="0 0 24 24"> | ||
| <path d="M12 0c-6.626 0-12 5.373-12 12 0 5.302 3.438 9.8 8.207 11.387.599.111.793-.261.793-.577v-2.234c-3.338.726-4.033-1.416-4.033-1.416-.546-1.387-1.333-1.756-1.333-1.756-1.089-.745.083-.729.083-.729 1.205.084 1.839 1.237 1.839 1.237 1.07 1.834 2.807 1.304 3.492.997.107-.775.418-1.305.762-1.604-2.665-.305-5.467-1.334-5.467-5.931 0-1.311.469-2.381 1.236-3.221-.124-.303-.535-1.524.117-3.176 0 0 1.008-.322 3.301 1.23.957-.266 1.983-.399 3.003-.404 1.02.005 2.047.138 3.006.404 2.291-1.552 3.297-1.23 3.297-1.23.653 1.653.242 2.874.118 3.176.77.84 1.235 1.911 1.235 3.221 0 4.609-2.807 5.624-5.479 5.921.43.372.823 1.102.823 2.222v3.293c0 .319.192.694.801.576 4.765-1.589 8.199-6.086 8.199-11.386 0-6.627-5.373-12-12-12z"/> | ||
| </svg> | ||
| View on GitHub | ||
| </a> |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to external link.
External links with target="_blank" should include rel="noopener noreferrer" to prevent potential tabnabbing attacks and improve performance.
🔎 Proposed fix
<a href="https://github.com/BuildWithHussain/nano_press"
target="_blank"
+ rel="noopener noreferrer"
class="inline-flex items-center justify-center px-8 py-4 text-lg font-semibold text-gray-700 bg-white border-2 border-gray-300 rounded-lg hover:border-gray-400 transition-all duration-200 shadow-md hover:shadow-lg">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a href="https://github.com/BuildWithHussain/nano_press" | |
| target="_blank" | |
| class="inline-flex items-center justify-center px-8 py-4 text-lg font-semibold text-gray-700 bg-white border-2 border-gray-300 rounded-lg hover:border-gray-400 transition-all duration-200 shadow-md hover:shadow-lg"> | |
| <svg xmlns="http://www.w3.org/2000/svg" class="w-5 h-5 mr-2" fill="currentColor" viewBox="0 0 24 24"> | |
| <path d="M12 0c-6.626 0-12 5.373-12 12 0 5.302 3.438 9.8 8.207 11.387.599.111.793-.261.793-.577v-2.234c-3.338.726-4.033-1.416-4.033-1.416-.546-1.387-1.333-1.756-1.333-1.756-1.089-.745.083-.729.083-.729 1.205.084 1.839 1.237 1.839 1.237 1.07 1.834 2.807 1.304 3.492.997.107-.775.418-1.305.762-1.604-2.665-.305-5.467-1.334-5.467-5.931 0-1.311.469-2.381 1.236-3.221-.124-.303-.535-1.524.117-3.176 0 0 1.008-.322 3.301 1.23.957-.266 1.983-.399 3.003-.404 1.02.005 2.047.138 3.006.404 2.291-1.552 3.297-1.23 3.297-1.23.653 1.653.242 2.874.118 3.176.77.84 1.235 1.911 1.235 3.221 0 4.609-2.807 5.624-5.479 5.921.43.372.823 1.102.823 2.222v3.293c0 .319.192.694.801.576 4.765-1.589 8.199-6.086 8.199-11.386 0-6.627-5.373-12-12-12z"/> | |
| </svg> | |
| View on GitHub | |
| </a> | |
| <a href="https://github.com/BuildWithHussain/nano_press" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| class="inline-flex items-center justify-center px-8 py-4 text-lg font-semibold text-gray-700 bg-white border-2 border-gray-300 rounded-lg hover:border-gray-400 transition-all duration-200 shadow-md hover:shadow-lg"> | |
| <svg xmlns="http://www.w3.org/2000/svg" class="w-5 h-5 mr-2" fill="currentColor" viewBox="0 0 24 24"> | |
| <path d="M12 0c-6.626 0-12 5.373-12 12 0 5.302 3.438 9.8 8.207 11.387.599.111.793-.261.793-.577v-2.234c-3.338.726-4.033-1.416-4.033-1.416-.546-1.387-1.333-1.756-1.333-1.756-1.089-.745.083-.729.083-.729 1.205.084 1.839 1.237 1.839 1.237 1.07 1.834 2.807 1.304 3.492.997.107-.775.418-1.305.762-1.604-2.665-.305-5.467-1.334-5.467-5.931 0-1.311.469-2.381 1.236-3.221-.124-.303-.535-1.524.117-3.176 0 0 1.008-.322 3.301 1.23.957-.266 1.983-.399 3.003-.404 1.02.005 2.047.138 3.006.404 2.291-1.552 3.297-1.23 3.297-1.23.653 1.653.242 2.874.118 3.176.77.84 1.235 1.911 1.235 3.221 0 4.609-2.807 5.624-5.479 5.921.43.372.823 1.102.823 2.222v3.293c0 .319.192.694.801.576 4.765-1.589 8.199-6.086 8.199-11.386 0-6.627-5.373-12-12-12z"/> | |
| </svg> | |
| View on GitHub | |
| </a> |
🤖 Prompt for AI Agents
In nano_press/templates/components/landing/hero.html around lines 25 to 32 the
external GitHub anchor uses target="_blank" but is missing rel="noopener
noreferrer"; update the <a> tag to include rel="noopener noreferrer" to prevent
tabnabbing and improve performance while keeping the existing attributes intact.
| {% set user_image = frappe.db.get_value("User", frappe.session.user, "user_image") or "/assets/frappe/images/default-avatar.png" %} | ||
| {% set full_name = frappe.utils.get_fullname(frappe.session.user) if frappe.utils.get_fullname else frappe.session.user %} |
There was a problem hiding this comment.
Conditional check may not work as intended.
The condition if frappe.utils.get_fullname checks if the function exists, not if it returns a non-empty value. The fallback to frappe.session.user would only trigger if get_fullname is undefined, not if it returns an empty string.
🔎 Proposed fix
{% set user_image = frappe.db.get_value("User", frappe.session.user, "user_image") or "/assets/frappe/images/default-avatar.png" %}
-{% set full_name = frappe.utils.get_fullname(frappe.session.user) if frappe.utils.get_fullname else frappe.session.user %}
+{% set full_name = frappe.utils.get_fullname(frappe.session.user) or frappe.session.user %}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {% set user_image = frappe.db.get_value("User", frappe.session.user, "user_image") or "/assets/frappe/images/default-avatar.png" %} | |
| {% set full_name = frappe.utils.get_fullname(frappe.session.user) if frappe.utils.get_fullname else frappe.session.user %} | |
| {% set user_image = frappe.db.get_value("User", frappe.session.user, "user_image") or "/assets/frappe/images/default-avatar.png" %} | |
| {% set full_name = frappe.utils.get_fullname(frappe.session.user) or frappe.session.user %} |
🤖 Prompt for AI Agents
In nano_press/templates/includes/sidebar.html around lines 1-2, the current
conditional uses "if frappe.utils.get_fullname" which only checks existence of
the function, not its return value; replace this with calling get_fullname and
falling back on frappe.session.user when the result is empty or falsy (e.g.,
assign full_name = frappe.utils.get_fullname(frappe.session.user) or
frappe.session.user) so the fallback triggers when get_fullname returns an empty
string or None.
| </head> | ||
|
|
||
| <style> | ||
| [x-cloak] { | ||
| display: none !important; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
Move <style> block inside <head>.
The <style> block for x-cloak is placed between </head> and <body>, which is invalid HTML structure. Move it inside the <head> element, preferably after the stylesheet link.
🔎 Proposed fix
<link rel="stylesheet" href="/assets/nano_press/css/tailwind.css">
+ <style>
+ [x-cloak] {
+ display: none !important;
+ }
+ </style>
+
<script defer src="/assets/nano_press/js/vendor/alpinejs/core.js"></script>
{% block head %} {% endblock %}
</head>
-
-<style>
- [x-cloak] {
- display: none !important;
- }
-</style>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| </head> | |
| <style> | |
| [x-cloak] { | |
| display: none !important; | |
| } | |
| </style> | |
| <link rel="stylesheet" href="/assets/nano_press/css/tailwind.css"> | |
| <style> | |
| [x-cloak] { | |
| display: none !important; | |
| } | |
| </style> | |
| <script defer src="/assets/nano_press/js/vendor/alpinejs/core.js"></script> | |
| {% block head %} {% endblock %} | |
| </head> | |
| <body> |
🤖 Prompt for AI Agents
In nano_press/templates/layout_public.html around lines 22 to 28 the inline
<style> block defining [x-cloak] is placed after the closing </head>, which is
invalid; move that entire <style> block inside the <head> element (preferably
immediately after the stylesheet link) so it becomes part of the document head
and remove it from between </head> and <body> to restore valid HTML structure.
| {% include "templates/includes/footer.html" %} | ||
| </body> | ||
| {% block script %} {% endblock %} | ||
| </html> |
There was a problem hiding this comment.
Move script block inside <body>.
The {% block script %} is placed after </body>, which is invalid HTML structure. Move it inside the <body> element, after the footer include.
🔎 Proposed fix
{% include "templates/includes/footer.html" %}
+ {% block script %} {% endblock %}
</body>
-{% block script %} {% endblock %}
</html>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {% include "templates/includes/footer.html" %} | |
| </body> | |
| {% block script %} {% endblock %} | |
| </html> | |
| {% include "templates/includes/footer.html" %} | |
| {% block script %} {% endblock %} | |
| </body> | |
| </html> |
🤖 Prompt for AI Agents
In nano_press/templates/layout_public.html around lines 35 to 38, the {% block
script %} is placed after </body>, producing invalid HTML; move the block inside
the <body> element immediately after the {% include
"templates/includes/footer.html" %} line and before the </body> tag so scripts
are rendered inside the document body, then close </body> and </html> as before.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
nano_press/nano_press/doctype/app_version/app_version.py (1)
8-11: Consider adding explicit validation for edge cases.The
autonamemethod relies on the truthiness ofself.version. While the field is marked as required in the DocType schema, adding explicit validation would make the code more defensive against edge cases like empty strings or whitespace-only values.🔎 Proposed defensive validation
def autoname(self): - if self.version: + if self.version and self.version.strip(): self.scrubbed_version = self.version.lower().replace(" ", "-") self.name = self.scrubbed_version + else: + frappe.throw("Version cannot be empty")nano_press/templates/components/core/apps.html (1)
238-268: Consider refactoring validation for maintainability.The validation logic is comprehensive and correct, but the multiple
ifblocks make it verbose. Consider extracting validation rules into a schema or helper functions for better maintainability.🔎 Example validation refactor
+validateCustomAppForm() { + const rules = { + name: { required: true, message: 'App name is required.' }, + githubUrl: { + required: true, + message: 'GitHub URL is required.', + pattern: /^https:\/\//, + patternMessage: 'GitHub URL must start with https://' + }, + branch: { required: true, message: 'Branch is required.' }, + token: { + required: this.appType === 'private', + message: 'GitHub Token is required for private repositories.' + } + }; + + let hasError = false; + for (const [field, rule] of Object.entries(rules)) { + if (rule.required && !this.localCustomAppForm[field]) { + this.formErrors[field] = rule.message; + hasError = true; + } else if (rule.pattern && !rule.pattern.test(this.localCustomAppForm[field])) { + this.formErrors[field] = rule.patternMessage; + hasError = true; + } + } + return !hasError; +}, + addCustomApp() { this.formErrors = { name: '', githubUrl: '', token: '', branch: '' }; - let hasError = false; - // ... rest of validation ... - if (hasError) { + if (!this.validateCustomAppForm()) { return; }nano_press/nano_press/doctype/custom_image/custom_image.py (2)
79-100: Clarify exception handling strategy.The exception handler at lines 96-100 performs rollback, updates status, logs the error, then re-raises the exception. This pattern is unclear:
- The status update at line 81 already committed via
_update_status(line 104), so the rollback won't undo it- Re-raising the exception means callers still see the failure
- The rollback and status update may not provide the intended safety net
Consider either:
- Fully handle the exception and return an error status (don't re-raise), or
- Remove the exception handler and let failures propagate naturally (move cleanup to a
finallyblock if needed)
156-188: Consider HTML escaping in email messages.The broad exception handling at line 187 is appropriate for notification code (similar to line 153).
However, the HTML email messages at lines 164-178 use f-strings with user-controlled values (
self.image_name,self.image_tag, etc.) without escaping. While less critical in email context than web XSS, escaping HTML entities would improve correctness and prevent potential rendering issues.🔎 Optional: Add HTML escaping
+from html import escape def _send_email_notification(self, status): try: recipient = frappe.db.get_value("User", self.owner, "email") if not recipient or "@" not in recipient: return if status == "success": subject = _("Custom Image Build Successful - {0}").format(self.image_name) message = f""" - <p>Your custom Docker image <strong>{self.image_name}</strong> has been built successfully.</p> - <p><strong>Image Tag:</strong> {self.image_tag}</p> - <p><strong>Server:</strong> {self.server_name}</p> - <p><strong>Frappe Version:</strong> {self.frappe_version}</p> - <p><strong>Build Duration:</strong> {self.build_duration or 0} seconds</p> + <p>Your custom Docker image <strong>{escape(self.image_name)}</strong> has been built successfully.</p> + <p><strong>Image Tag:</strong> {escape(self.image_tag)}</p> + <p><strong>Server:</strong> {escape(self.server_name)}</p> + <p><strong>Frappe Version:</strong> {escape(str(self.frappe_version))}</p> + <p><strong>Build Duration:</strong> {escape(str(self.build_duration or 0))} seconds</p> """ else: subject = _("Custom Image Build Failed - {0}").format(self.image_name) message = f""" <p>Your custom Docker image build encountered an error.</p> - <p><strong>Image Name:</strong> {self.image_name}</p> - <p><strong>Server:</strong> {self.server_name}</p> + <p><strong>Image Name:</strong> {escape(self.image_name)}</p> + <p><strong>Server:</strong> {escape(self.server_name)}</p> <p>Please check the build log for more details.</p> """
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
nano_press/nano_press/doctype/app_version/__init__.pynano_press/nano_press/doctype/app_version/app_version.jsonnano_press/nano_press/doctype/app_version/app_version.pynano_press/nano_press/doctype/apps/apps.jsonnano_press/nano_press/doctype/custom_image/custom_image.jsonnano_press/nano_press/doctype/custom_image/custom_image.pynano_press/templates/components/core/apps.htmlnano_press/templates/components/core/server.htmlnano_press/www/self-host.htmlnano_press/www/self_host.py
🧰 Additional context used
🪛 GitHub Actions: Linters
nano_press/nano_press/doctype/custom_image/custom_image.py
[error] 104-104: Semgrep blocking finding: frappe-manual-commit. Manually committing a transaction is highly discouraged. If manual commit is required then add a comment explaining why and '# nosemgrep' on the same line.
🪛 Ruff (0.14.10)
nano_press/nano_press/doctype/custom_image/custom_image.py
153-153: Do not catch blind exception: Exception
(BLE001)
187-187: Do not catch blind exception: Exception
(BLE001)
212-212: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (21)
nano_press/www/self-host.html (1)
34-41: LGTM! Version string standardization.The change from
'Version-15'to'version-15'aligns with the new App Version DocType'sscrubbed_versionformat, ensuring consistency across the application.nano_press/nano_press/doctype/custom_image/custom_image.json (1)
31-36: LGTM! Improved version management.Changing the
frappe_versionfield to link to the App Version DocType improves maintainability by centralizing version definitions and eliminating hardcoded version strings.nano_press/www/self_host.py (3)
6-12: LGTM! Enhanced context for UI features.The additions to
get_contextproperly expose server and version data to the template, supporting the new server selection and version management features.
15-21: LGTM! Secure server filtering.The
get_user_serversfunction correctly filters servers by owner, ensuring users can only access their own server records.
24-37: LGTM! Expanded app and version data.The updated
get_appsand newget_frappe_versionsfunctions properly expose the additional fields needed for the enhanced UI, including app logos and frappe app identification.nano_press/nano_press/doctype/app_version/app_version.json (1)
1-48: LGTM! Well-structured DocType.The App Version DocType is properly configured with a unique constraint on the version field and appropriate permissions. The "By script" naming rule aligns with the
autonamemethod implementation.nano_press/nano_press/doctype/apps/apps.json (1)
93-110: LGTM! Enhanced app metadata fields.The new fields effectively support custom app management and visual branding. The
is_customfield with default "0" properly distinguishes between Frappe and custom apps, and theapp_logointegration withimage_fieldenables proper image handling.nano_press/templates/components/core/server.html (2)
14-33: LGTM! Server selection UI.The server selection dropdown is well-implemented with conditional rendering and displays relevant server information (name, IP, status). The UI gracefully handles the case when no servers are available.
164-187: LGTM! Robust server selection logic.The
onServerSelectmethod properly:
- Finds and populates server details into the installer state
- Handles verification status appropriately
- Uses defensive coding (checking
toastexistence, converting port to string)- Integrates cleanly with the existing verification flow
nano_press/templates/components/core/apps.html (2)
4-17: LGTM! Version selection updated.The Frappe version selection correctly uses the new
frappe_versionsdata, displaying the user-friendlyversionwhile using the normalizedscrubbed_versionfor state management.
19-98: LGTM! Well-organized app sections.The separation of Frappe apps and custom apps provides a clear user experience. The use of computed getters (
frappeApps,customApps) keeps the logic clean, and the consistent UI treatment (logos/initials, branch display) enhances usability.nano_press/nano_press/doctype/custom_image/custom_image.py (10)
15-17: LGTM!The refactored
before_savehook cleanly delegates to private methods for generating the apps JSON and setting the image tag.
23-47: LGTM!The method properly validates app configuration, uses localized error messages via
_(), and delegates URL building and sorting to focused helper methods.
49-51: LGTM!Clean base64 encoding wrapper.
106-119: LGTM!The success handler correctly calculates build duration, atomically updates multiple fields, and notifies the user.
121-124: LGTM!Clean failure handling with status update, notification, and error logging.
126-128: LGTM!Clean separation of notification channels.
130-154: LGTM!The broad exception handling at line 153 (flagged by Ruff) is appropriate here—notification failures should be logged without disrupting the build flow. The error is properly logged at line 154.
190-200: LGTM!Properly enqueues the build job with appropriate queue settings and
enqueue_after_commit=Trueto ensure the document is saved before the job runs.
202-213: LGTM!The broad exception handling at line 212 is appropriate—this preview endpoint should return graceful error responses rather than throwing exceptions. The error is captured and returned in the response dict.
272-279: LGTM!Clean status getter with appropriate conditional return of
image_tagonly when build is complete.
| def _update_status(self, status): | ||
| frappe.db.set_value("Custom Image", self.name, "build_status", status, update_modified=False) | ||
| frappe.db.commit() |
There was a problem hiding this comment.
Pipeline failure: manual commit requires justification.
Semgrep flagged line 104 for manually committing the transaction without explanation. Manual commits are discouraged in Frappe due to potential data consistency issues.
🔎 Proposed fix with justification comment
def _update_status(self, status):
frappe.db.set_value("Custom Image", self.name, "build_status", status, update_modified=False)
- frappe.db.commit()
+ # Manual commit needed: build runs in background job, status must be visible to UI immediately # nosemgrep
+ frappe.db.commit()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _update_status(self, status): | |
| frappe.db.set_value("Custom Image", self.name, "build_status", status, update_modified=False) | |
| frappe.db.commit() | |
| def _update_status(self, status): | |
| frappe.db.set_value("Custom Image", self.name, "build_status", status, update_modified=False) | |
| # Manual commit needed: build runs in background job, status must be visible to UI immediately # nosemgrep | |
| frappe.db.commit() |
🧰 Tools
🪛 GitHub Actions: Linters
[error] 104-104: Semgrep blocking finding: frappe-manual-commit. Manually committing a transaction is highly discouraged. If manual commit is required then add a comment explaining why and '# nosemgrep' on the same line.
🤖 Prompt for AI Agents
In nano_press/nano_press/doctype/custom_image/custom_image.py around lines
102-104, the explicit call to frappe.db.commit() (line 104) must be removed
because manual transaction commits are discouraged; modify the method to only
call frappe.db.set_value(...) and drop the frappe.db.commit() call, and if a
commit is truly required for a specific edge case, replace the removal with a
short justification comment and perform the commit in the caller/context where
transaction boundaries are explicit and documented.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nano_press/templates/components/core/apps.html (1)
224-236: 🔴 CRITICAL: GitHub tokens still stored in localStorage.The security issue flagged in the previous review remains unresolved. The
addCustomAppmethod stores the GitHub token inlocalCustomApps, which is then synced to$store.installer.customAppsviasyncToStore()(line 235). This persists tokens to localStorage, creating a significant security risk:
- localStorage is accessible to any JavaScript code, including third-party scripts
- XSS vulnerabilities could expose these tokens
- GitHub Personal Access Tokens grant significant permissions
Required action: Implement server-side token storage as recommended in the previous review.
🔎 Recommended approach (from previous review)
- Send the token to the backend immediately via a secure API call
- Store only a token reference ID (e.g., encrypted token ID) in localStorage
- Retrieve the actual token server-side when needed for operations
- Implement token encryption at rest on the server
Alternatively, avoid persisting tokens entirely and require users to re-enter them when needed.
🧹 Nitpick comments (3)
nano_press/nano_press/doctype/server/server.py (1)
244-244: Translation wrapper applied correctly, but consider consistency.The error message is now translatable. However, lines 133-139 use
frappe._()while this line uses_(). Both work, but using a consistent pattern throughout the file would improve maintainability.💡 Optional: unify translation helper usage
Consider standardizing on one pattern throughout the file:
- frappe.throw(frappe._("Traefik domain is required for Traefik deployment")) + frappe.throw(_("Traefik domain is required for Traefik deployment"))Apply similarly to lines 135, 137, and 139.
nano_press/nano_press/utils/remote_builder.py (1)
11-11: LGTM! Validation messages properly internationalized.All validation error messages in the build flow are now translatable. The implementation is clean and consistent.
Consider also wrapping the error message at line 65 for consistency:
frappe.throw(_("Server {0} is not verified. Please verify server connectivity first.").format(server_doc.name))Also applies to: 25-25, 51-60
nano_press/templates/components/core/apps.html (1)
229-229: Redundant default value for branch.The fallback
|| 'main'is redundant sincelocalCustomAppForm.branchalready defaults to'main'at line 215.🔎 Simplified code
- branch: this.localCustomAppForm.branch || 'main' + branch: this.localCustomAppForm.branch
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
nano_press/nano_press/doctype/frappe_site/frappe_site.pynano_press/nano_press/doctype/server/server.pynano_press/nano_press/utils/ansible/src/AnsibleRunner.pynano_press/nano_press/utils/remote_builder.pynano_press/public/js/script.jsnano_press/templates/components/core/apps.htmlnano_press/templates/includes/home/custom_app_modal.htmlnano_press/utils/ansible_runner.py
🚧 Files skipped from review as they are similar to previous changes (1)
- nano_press/public/js/script.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (7)
nano_press/nano_press/doctype/server/server.py (1)
8-8: LGTM! Translation import added correctly.The import follows Frappe's standard i18n pattern.
nano_press/nano_press/doctype/frappe_site/frappe_site.py (1)
7-7: Excellent i18n implementation!All error messages are properly wrapped for translation, and line 73 correctly uses
.format()for the placeholder instead of f-strings, which is the recommended pattern for gettext-style translation.Also applies to: 71-76
nano_press/utils/ansible_runner.py (1)
4-4: LGTM! API validation messages now translatable.The translation wrappers are correctly applied to user-facing validation messages at the API boundary.
Also applies to: 34-34, 106-106
nano_press/nano_press/utils/ansible/src/AnsibleRunner.py (1)
17-17: LGTM! Connection validation errors now translatable.The translation wrappers are properly applied to connection validation errors in both
run_playbookandrun_pingmethods, maintaining consistency across the class.Also applies to: 84-84, 144-144, 250-250
nano_press/templates/includes/home/custom_app_modal.html (1)
7-59: LGTM! Well-structured form with proper validation and accessibility.The modal form is cleanly implemented with:
- Proper ARIA attributes for the tabbed interface
- HTML5 validation (required, pattern, type constraints)
- Conditional token field based on app type
- Clear labels and helpful placeholders
nano_press/templates/components/core/apps.html (2)
198-204: Good use of computed getters to separate Frappe and custom apps.The getters cleanly split the available apps based on the
frappefield, making the template logic simpler and more maintainable.
252-260: Good state synchronization pattern.The
syncToStore()method provides a clean way to keep local component state in sync with the central store, and it's properly called after each state mutation (inremoveCustomApp,toggleApp,selectFrappeVersion, andaddCustomApp).
…nagement Security Fixes: - Change pat_token field from Data to Password for encrypted storage - Add register_custom_app() API to securely handle token storage - Tokens no longer stored in localStorage (only reference IDs persist) - Tokens cleared from client-side immediately after backend POST - Add delete_custom_app() API to properly remove apps from backend - Tokens encrypted at rest using Frappe's Password field
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nano_press/templates/components/core/apps.html (1)
237-237: Case sensitivity concern for app names.Converting
app_nameto lowercase may cause issues if the actual repository name or app package name is case-sensitive. This was flagged in a previous review. Consider preserving the original case or validating the normalized name against the actual repository structure.
🧹 Nitpick comments (2)
nano_press/nano_press/doctype/custom_image/custom_image.py (1)
153-154: Add specific exception types for better error handling.The blind
Exceptioncatches in notification methods could mask unexpected errors. While notification failures shouldn't break the build flow, catching specific exceptions (e.g.,frappe.ValidationError,SMTPException) would improve debugging.🔎 Proposed refinement
except Exception: - frappe.log_error(title=_("Build Notification Failed")) + frappe.log_error(title=_("Build Notification Failed"), message=frappe.get_traceback())Apply similar change to line 187-188.
Also applies to: 187-188
nano_press/api.py (1)
91-92: Add URL validation for github_url.The endpoint validates presence of
github_urlbut doesn't verify it's a valid GitHub URL format. Consider adding pattern validation to ensure it starts withhttps://github.com/to catch input errors early.🔎 Proposed validation
if not app_name or not github_url or not branch: frappe.throw(_("app_name, github_url, and branch are required")) + + if not github_url.startswith("https://github.com/"): + frappe.throw(_("github_url must be a valid GitHub repository URL starting with https://github.com/"))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
nano_press/api.pynano_press/nano_press/doctype/apps/apps.jsonnano_press/nano_press/doctype/custom_image/custom_image.pynano_press/templates/components/core/apps.html
🧰 Additional context used
🧬 Code graph analysis (1)
nano_press/nano_press/doctype/custom_image/custom_image.py (2)
nano_press/nano_press/utils/ansible/src/AnsibleRunner.py (1)
run_playbook(60-120)nano_press/utils/ansible_runner.py (1)
run_playbook(11-84)
🪛 Ruff (0.14.10)
nano_press/api.py
128-128: Do not catch blind exception: Exception
(BLE001)
155-155: Do not catch blind exception: Exception
(BLE001)
nano_press/nano_press/doctype/custom_image/custom_image.py
153-153: Do not catch blind exception: Exception
(BLE001)
187-187: Do not catch blind exception: Exception
(BLE001)
212-212: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (7)
nano_press/nano_press/doctype/apps/apps.json (2)
57-61: LGTM! Security improvement for token handling.The change from
DatatoPasswordfieldtype forpat_tokenis a significant security enhancement, ensuring tokens are encrypted at rest. The conditional display logic (depends_on: "eval: doc.is_custom == 1") and added description properly guide users on when tokens are required.
93-110: LGTM! Well-structured field additions.The new fields (
app_logo,is_custom,column_break_eisq) follow Frappe conventions properly. Theimage_fieldlinkage on Line 110 correctly associates the DocType with theapp_logofield for image handling.nano_press/api.py (1)
120-120: Manual commit appropriately documented.The manual commit on line 120 is properly justified with a nosemgrep comment for the API endpoint's transactional boundary. Same applies to line 151.
nano_press/templates/components/core/apps.html (4)
243-270: LGTM! Significant security improvement for token handling.The refactored custom app flow properly addresses the previous localStorage security concern. Tokens are now sent directly to the backend via the
register_custom_appAPI and never persisted in client storage. The flow correctly:
- Posts tokens immediately to the backend (line 244-253)
- Stores only the
app_reference_idreference (line 259)- Clears the form including the token field (line 274)
- Handles errors appropriately with rollback on the backend
This is a substantial security enhancement.
159-166: LGTM! Proper conditional validation for private apps.The token field is correctly marked as required only when
appType === 'private'using the:requiredbinding. This ensures private repositories have authentication while not burdening public repo users.
208-214: LGTM! Clean separation of Frappe and custom apps.The computed getters (
frappeAppsandcustomApps) provide a clean separation based on thefrappefield value, supporting the dual-section UI introduced in this PR.
280-306: LGTM! Proper cleanup with backend synchronization.The
removeCustomAppmethod correctly handles deletion by:
- Calling the backend API to delete the app record if it has an
app_reference_id- Providing user feedback via toast notifications
- Gracefully handling backend failures while still removing the local entry
- Syncing state after removal
- Fix Docker tag sanitization (handle special chars, leading dots/dashes) - Fix app sorting bug with index alignment - Optimize deployment vars to use pre-computed apps_json_base64 - Add server existence validation before build - Validate custom app entries require name and repo_url
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
nano_press/nano_press/doctype/custom_image/custom_image.py (1)
71-84: Critical: Index mismatch bug in app sorting remains unfixed.The method still assumes positional correspondence between
apps_configindices andapps_list, but_generate_apps_json(lines 37-38) skips entries whereapp_item.app_nameisNone. This causes index misalignment.Example of the bug:
apps_config=[item_no_name, itemA, itemB]- After
_generate_apps_json,apps_list=[urlA, urlB](only 2 items)- Line 82 with
i=1accessesapps_list[1](urlB) but pairs it with itemA's order- Result: incorrect app ordering and potentially missing apps
The previous review comment on this issue recommended rebuilding
apps_listin sorted order rather than relying on index matching.🔎 Proposed fix: rebuild apps_list in correct order
def _sort_apps_by_order(self, apps_list): - app_order_map = {} + # Build order map for all apps + order_map = {} for item in self.apps_config: if item.app_name: - order = frappe.db.get_value("Apps", item.app_name, "order") or 999 - app_order_map[item.app_name] = order - - sorted_apps = [] - for i, app_item in enumerate(self.apps_config): - if app_item.app_name and i < len(apps_list): - order = app_order_map.get(app_item.app_name, 999) - sorted_apps.append((apps_list[i], order)) - - return [app for app, _ in sorted(sorted_apps, key=lambda x: x[1])] + order_map[item.app_name] = frappe.db.get_value("Apps", item.app_name, "order") or 999 + + # Sort apps_config by order + sorted_config = sorted( + [item for item in self.apps_config if item.app_name], + key=lambda item: order_map.get(item.app_name, 999) + ) + + # Rebuild apps_list in sorted order + sorted_list = [] + for item in sorted_config: + app_doc = frappe.get_cached_doc("Apps", item.app_name) + sorted_list.append({ + "url": self._build_repo_url(app_doc), + "branch": app_doc.branch, + }) + + return sorted_listThis approach sorts
apps_configfirst, then regeneratesapps_listin the correct order, eliminating the index-based assumption.
🧹 Nitpick comments (3)
nano_press/nano_press/doctype/custom_image/custom_image.py (3)
19-29: Inconsistent character handling in sanitization.Line 23 replaces whitespace with
-, but line 24 removes other invalid characters entirely (empty string replacement). This creates inconsistent results:"My App@Name"becomes"my-appname"instead of"my-app-name".Consider replacing invalid characters with
-on line 24 to match the whitespace handling on line 23, ensuring consistent separator usage throughout the sanitized name.🔎 Proposed fix for consistency
def _set_image_tag(self): import re name = self.image_name.lower() name = re.sub(r"\s+", "-", name) - name = re.sub(r"[^a-z0-9_.-]", "", name) + name = re.sub(r"[^a-z0-9_.-]", "-", name) name = re.sub(r"[-_.]{2,}", "-", name) name = name.lstrip(".-") sanitized_name = name if name else "image" self.image_tag = f"{sanitized_name}:latest"
116-118: Add justification comment for manual commit.While the
nosemgrepdirective suppresses the linter, the code should include a brief comment explaining why manual commit is necessary here (e.g., "build runs in background job, status must be visible to UI immediately").🔎 Proposed improvement
def _update_status(self, status): frappe.db.set_value("Custom Image", self.name, "build_status", status, update_modified=False) - frappe.db.commit() # nosemgrep: frappe-semgrep-rules.rules.frappe-manual-commit + # Manual commit needed: build runs in background job, status must be visible to UI immediately + frappe.db.commit() # nosemgrep: frappe-semgrep-rules.rules.frappe-manual-commit
259-267: Strengthen custom app validation.Lines 263-266 validate that
app_nameandrepo_urlare present, but don't check for empty strings or whitespace-only values. An empty"githubUrl": ""would pass theif not repo_urlcheck but cause issues downstream.🔎 Proposed fix with stricter validation
for custom_app in custom_apps: app_name = custom_app.get("name") - repo_url = custom_app.get("githubUrl") + repo_url = (custom_app.get("githubUrl") or "").strip() - if not app_name or not repo_url: + if not app_name or not repo_url: frappe.throw( _("Custom app must have both 'name' and 'githubUrl' fields. Received: {0}").format(custom_app) )This ensures
githubUrlvalues like""or" "are properly rejected.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nano_press/nano_press/doctype/custom_image/custom_image.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Venkateshvenki404224
Repo: BuildWithHussain/nano_press PR: 22
File: nano_press/nano_press/doctype/custom_image/custom_image.py:246-257
Timestamp: 2026-01-04T15:17:52.551Z
Learning: In Nano Press's secure token flow, private app tokens are registered via the `register_custom_app()` API before `create_and_build_custom_image()` is called. The `custom_apps` payload passed to `create_and_build_custom_image()` does NOT contain tokens (only name, githubUrl, branch, app_reference_id). Apps with tokens already exist in the database when `create_and_build_custom_image()` runs, so the fallback doc creation (when `frappe.db.exists("Apps", app_name)` is False) only applies to public apps and correctly sets `is_public=1`.
📚 Learning: 2026-01-04T15:17:52.551Z
Learnt from: Venkateshvenki404224
Repo: BuildWithHussain/nano_press PR: 22
File: nano_press/nano_press/doctype/custom_image/custom_image.py:246-257
Timestamp: 2026-01-04T15:17:52.551Z
Learning: In Nano Press's secure token flow, private app tokens are registered via the `register_custom_app()` API before `create_and_build_custom_image()` is called. The `custom_apps` payload passed to `create_and_build_custom_image()` does NOT contain tokens (only name, githubUrl, branch, app_reference_id). Apps with tokens already exist in the database when `create_and_build_custom_image()` runs, so the fallback doc creation (when `frappe.db.exists("Apps", app_name)` is False) only applies to public apps and correctly sets `is_public=1`.
Applied to files:
nano_press/nano_press/doctype/custom_image/custom_image.py
🪛 Ruff (0.14.10)
nano_press/nano_press/doctype/custom_image/custom_image.py
167-167: Do not catch blind exception: Exception
(BLE001)
201-201: Do not catch blind exception: Exception
(BLE001)
226-226: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (7)
nano_press/nano_press/doctype/custom_image/custom_image.py (7)
31-55: LGTM!The apps JSON generation logic is well-structured with proper validation, efficient caching via
get_cached_doc, and appropriate localization of error messages.
61-69: Verify that PAT tokens in URLs won't leak in logs.The previous review flagged embedding authentication tokens in repository URLs as a security risk (tokens can appear in Ansible logs, build logs, and process listings). Although marked as addressed in commit f048dd1, tokens are still being embedded in HTTPS URLs on line 67.
Please confirm:
- This is the intended approach after security review
- Build logs and Ansible playbooks properly sanitize URLs containing credentials
- Git credential helpers or environment variables weren't viable alternatives
86-91: LGTM! Previous redundancy issue fixed.The method now correctly uses the pre-computed
self.apps_json_base64value instead of regenerating it, addressing the performance concern from the previous review.
140-202: LGTM! Notification handling is robust.The notification methods are well-structured with proper separation of concerns and defensive exception handling. Catching broad
Exceptiontypes here is appropriate since notification failures should be logged but not disrupt the build process.
204-227: LGTM! Public API methods are well-implemented.The
enqueue_build_custom_imagemethod properly queues long-running builds, andpreview_apps_json_for_formprovides safe preview functionality with appropriate error handling.
268-280: LGTM! Apps doc creation is correct for the secure flow.The
is_public=1flag is appropriate here. Based on learnings, private apps with tokens are already registered in the database via theregister_custom_app()API before this function is called. This fallback creation only executes for public apps that weren't pre-registered, makingis_public=1the correct default.Based on learnings, this implements the secure token flow correctly.
294-302: LGTM! Build status endpoint is clean and appropriate.The function provides a concise status snapshot with conditional inclusion of
image_tagonly when the build is complete.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
nano_press/nano_press/doctype/custom_image/custom_image.py (1)
116-118: Add explanatory comment for manual commit.The
nosemgrepdirective is present, but the past review requested an inline justification explaining why the manual commit is necessary.🔎 Suggested addition
def _update_status(self, status): frappe.db.set_value("Custom Image", self.name, "build_status", status, update_modified=False) - frappe.db.commit() # nosemgrep: frappe-semgrep-rules.rules.frappe-manual-commit + # Manual commit needed: build runs in background job, status must be visible to UI immediately + frappe.db.commit() # nosemgrep: frappe-semgrep-rules.rules.frappe-manual-commitnano_press/templates/components/core/apps.html (2)
235-279: Consider adding duplicate app name validation.The
addCustomAppmethod doesn't check if an app with the same name already exists inlocalCustomAppsbefore adding. This could lead to duplicate entries in the UI if a user accidentally submits the same app twice.🔎 Suggested duplicate check
Add this check before line 242:
// Check for duplicate app names const isDuplicate = this.localCustomApps.some( app => app.name.toLowerCase() === appData.name.toLowerCase() ); if (isDuplicate) { if (typeof toast !== 'undefined') { toast(`App "${appData.name}" already exists`, { type: "warning" }); } return; }
21-69: LGTM! Consistent and accessible UI structure.The split between Frappe Apps and Custom Apps sections provides clear organization. The card structure with logo/initial fallbacks and consistent styling creates a polished user experience.
Optional: The inline
style="grid-template-columns: repeat(3, minmax(0, 1fr));"could be extracted to a CSS class for better maintainability if used in multiple places, but this is a minor point.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nano_press/nano_press/doctype/custom_image/custom_image.pynano_press/templates/components/core/apps.html
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Venkateshvenki404224
Repo: BuildWithHussain/nano_press PR: 22
File: nano_press/nano_press/doctype/custom_image/custom_image.py:246-257
Timestamp: 2026-01-04T15:17:52.551Z
Learning: In Nano Press's secure token flow, private app tokens are registered via the `register_custom_app()` API before `create_and_build_custom_image()` is called. The `custom_apps` payload passed to `create_and_build_custom_image()` does NOT contain tokens (only name, githubUrl, branch, app_reference_id). Apps with tokens already exist in the database when `create_and_build_custom_image()` runs, so the fallback doc creation (when `frappe.db.exists("Apps", app_name)` is False) only applies to public apps and correctly sets `is_public=1`.
📚 Learning: 2026-01-04T15:17:52.551Z
Learnt from: Venkateshvenki404224
Repo: BuildWithHussain/nano_press PR: 22
File: nano_press/nano_press/doctype/custom_image/custom_image.py:246-257
Timestamp: 2026-01-04T15:17:52.551Z
Learning: In Nano Press's secure token flow, private app tokens are registered via the `register_custom_app()` API before `create_and_build_custom_image()` is called. The `custom_apps` payload passed to `create_and_build_custom_image()` does NOT contain tokens (only name, githubUrl, branch, app_reference_id). Apps with tokens already exist in the database when `create_and_build_custom_image()` runs, so the fallback doc creation (when `frappe.db.exists("Apps", app_name)` is False) only applies to public apps and correctly sets `is_public=1`.
Applied to files:
nano_press/templates/components/core/apps.htmlnano_press/nano_press/doctype/custom_image/custom_image.py
🪛 Ruff (0.14.10)
nano_press/nano_press/doctype/custom_image/custom_image.py
167-167: Do not catch blind exception: Exception
(BLE001)
201-201: Do not catch blind exception: Exception
(BLE001)
226-226: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (10)
nano_press/nano_press/doctype/custom_image/custom_image.py (7)
15-29: LGTM! Image tag sanitization is comprehensive.The refactored
before_savehook and_set_image_tagmethod correctly handle Docker tag constraints by sanitizing the image name through multiple regex passes and providing a safe fallback.
31-51: LGTM! Efficient apps JSON generation.The use of
frappe.get_cached_doc()on Line 39 is a good optimization for performance, and the validation ensures apps have complete configuration before proceeding.
63-84: LGTM! Sorting logic correctly addresses previous index mismatch.The refactored
_sort_apps_by_ordermethod properly rebuilds the apps list in the correct order without relying on positional index alignment, resolving the previous concern about misalignment whenapp_nameisNone.
140-202: LGTM! Notification error handling is appropriate.The broad exception catching in
_send_realtime_notification(Line 167) and_send_email_notification(Line 201) is appropriate for graceful degradation—notification failures are logged but don't interrupt the build flow. The Ruff warnings are false positives in this context.
204-227: LGTM! Enqueue and preview methods are well-structured.The
enqueue_build_custom_imagemethod follows Frappe's standard enqueue pattern. The broad exception handling inpreview_apps_json_for_form(Line 226) is appropriate for a preview endpoint—it ensures the form receives structured error information rather than an unhandled exception. The Ruff warning is a false positive.
230-291: LGTM! Creation and validation logic is sound.The
create_and_build_custom_imagefunction properly validates server existence and status (Lines 235-242), ensures custom apps have required fields (Lines 263-266), and correctly creates fallback Apps docs withis_public=1(Lines 268-280). Based on learnings, this fallback only executes for public apps since private apps with tokens are pre-registered viaregister_custom_app(), making theis_public=1default appropriate.
294-302: LGTM! Build status endpoint is straightforward.The
get_build_statusfunction returns the essential build information with appropriate null handling for the image tag.nano_press/templates/components/core/apps.html (3)
280-306: LGTM! Solid error handling and state management.The
removeCustomAppmethod correctly:
- Attempts backend deletion only for apps that were registered (with
app_reference_id)- Handles backend errors gracefully without blocking local state cleanup
- Maintains consistency with the
addCustomApplogic- Syncs changes to the store
318-326: LGTM! Clean state synchronization pattern.The addition of
syncToStore()and its integration into state-changing methods (toggleApp,selectFrappeVersion,addCustomApp,removeCustomApp) ensures consistent state management between local component state and the global store.
208-214: LGTM! Clean separation of app types.The computed getters
frappeAppsandcustomAppsprovide a clear, reactive way to separate Frappe framework apps from custom apps based on thefrappeflag.
| <input type="url" x-model="localCustomAppForm.githubUrl" placeholder="https://github.com/user/repo" | ||
| required | ||
| pattern="https://.*" | ||
| title="URL must start with https://" | ||
| class="w-full px-3 py-2.5 bg-white border border-gray-300 rounded-md text-gray-900 outline-none transition-all duration-200 text-sm font-sans focus:border-gray-900"> | ||
| <p class="mt-1 text-xs text-gray-500">Must start with https://</p> |
There was a problem hiding this comment.
Strengthen GitHub URL validation pattern.
The current pattern https://.* accepts any HTTPS URL, not just GitHub repositories. This could allow invalid URLs that will fail during backend processing.
Consider using a more specific pattern that validates GitHub URLs:
🔎 Suggested validation pattern
- <input type="url" x-model="localCustomAppForm.githubUrl" placeholder="https://github.com/user/repo"
- required
- pattern="https://.*"
- title="URL must start with https://"
- class="w-full px-3 py-2.5 bg-white border border-gray-300 rounded-md text-gray-900 outline-none transition-all duration-200 text-sm font-sans focus:border-gray-900">
- <p class="mt-1 text-xs text-gray-500">Must start with https://</p>
+ <input type="url" x-model="localCustomAppForm.githubUrl" placeholder="https://github.com/user/repo"
+ required
+ pattern="https://github\.com/.+"
+ title="Must be a valid GitHub repository URL (https://github.com/user/repo)"
+ class="w-full px-3 py-2.5 bg-white border border-gray-300 rounded-md text-gray-900 outline-none transition-all duration-200 text-sm font-sans focus:border-gray-900">
+ <p class="mt-1 text-xs text-gray-500">Must be a GitHub repository URL</p>🤖 Prompt for AI Agents
In nano_press/templates/components/core/apps.html around lines 151-156, the
input pattern `https://.*` is too permissive for GitHub repo URLs; replace it
with a stricter regex that only allows GitHub repository URLs (for example a
pattern that enforces the host and user/repo path, e.g. starting with
https://github.com/, followed by a non-empty user and repo segment, optionally
ending with .git or a trailing slash). Update the title and helper text to
reflect the stronger validation (e.g., "URL must be a GitHub repository like
https://github.com/user/repo"). Ensure the new pattern is anchored and escapes
dots so it validates client-side before submission.
|
|
||
| localSelectedApps: [], | ||
| localFrappeVersion: 'Version-15', | ||
| localFrappeVersion: 'version-15', |
There was a problem hiding this comment.
Hardcoded Frappe version default may not exist in available versions.
The default value 'version-15' might not exist in frappeVersionOptions if the backend doesn't provide it. This would leave the user with an invalid selection.
Consider defaulting to the first available version or the most recent version from frappeVersionOptions:
🔎 Suggested dynamic default
- localFrappeVersion: 'version-15',
+ localFrappeVersion: '',Then update the init() method:
init() {
this.localSelectedApps = [...this.$store.installer.selectedApps];
- this.localFrappeVersion = this.$store.installer.frappeVersion;
+ this.localFrappeVersion = this.$store.installer.frappeVersion ||
+ (this.frappeVersionOptions.length > 0 ? this.frappeVersionOptions[0].scrubbed_version : '');
this.localCustomApps = [...this.$store.installer.customApps];
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In nano_press/templates/components/core/apps.html around line 217, the
localFrappeVersion is hardcoded to 'version-15', which may be absent from
frappeVersionOptions causing an invalid default; change the component to set
localFrappeVersion dynamically at initialization by selecting
frappeVersionOptions[0] or the newest/most appropriate entry (e.g., last or
based on a version sort) when the options array is present, and update init() to
assign this computed default only if the current value is missing or not found
in frappeVersionOptions.
Implement URL-based deployment tracking to preserve progress on page refresh. When user refreshes during deployment, polling automatically resumes and continues from current status. Removes blocking image build wait.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @nano_press/nano_press/doctype/frappe_site/frappe_site.json:
- Line 104: get_docker_image() currently returns custom_img.image_tag without
validating build_status, allowing deployments to use images that are still
"Building" or failed; update get_docker_image() to check custom_img.build_status
and only return custom_img.image_tag when build_status == "Built", otherwise
raise a user-facing error (or abort) indicating the custom image build is not
complete or failed so deployments cannot proceed with incomplete images.
In @nano_press/templates/components/core/summary.html:
- Around line 567-589: The summary view can be hidden because
$store.installer.currentStep isn't set when resuming a completed deployment;
update resumeCompletedDeployment (or init) to explicitly set
this.$store.installer.currentStep = 4 (or the step that shows the summary)
before setting deployComplete/deployStep so the installer store reflects the
summary state when ?complete=true&site=... is visited; modify the
resumeCompletedDeployment function (and/or init) to set currentStep early, then
continue fetching site data and credentials.
- Around line 80-99: The step indicator circles use 'text-gray-400' for the
current step (deployStep === n), which does nothing for a filled circle; update
each indicator div's :class ternary (the ones with "w-4 h-4 rounded-full
shrink-0") so that the middle branch (deployStep === n) applies a visible style
like 'bg-gray-500' or a ring/animation e.g. 'ring-2 ring-gray-900' or
'animate-pulse' instead of 'text-gray-400'; apply this change to every indicator
expression that currently uses (deployStep === X ? 'text-gray-400' :
'bg-gray-300') so the current step is visually distinct from incomplete steps.
- Around line 468-565: The polling interval created in resumeDeployment is
stored in a local const pollInterval and never saved on the component, so it
keeps running after navigation; change resumeDeployment to assign the interval
id to a component property (e.g., this.pollIntervalId) instead of the local
pollInterval, update every clearInterval(pollInterval) call to
clearInterval(this.pollIntervalId) and set this.pollIntervalId = null after
clearing, add pollIntervalId: null to the component data, and implement a
destroy() lifecycle method that checks and clears this.pollIntervalId to ensure
the interval is cleaned up when the component is destroyed.
🧹 Nitpick comments (1)
nano_press/templates/components/core/summary.html (1)
472-564: Consider adding a maximum poll duration or attempt limit.The polling continues indefinitely until a terminal state is reached. If the server gets stuck in an intermediate state, this will poll forever. Consider adding a timeout (e.g., 30 minutes) or max attempts to prevent indefinite polling.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nano_press/nano_press/doctype/frappe_site/frappe_site.jsonnano_press/templates/components/core/summary.html
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Venkateshvenki404224
Repo: BuildWithHussain/nano_press PR: 22
File: nano_press/nano_press/doctype/custom_image/custom_image.py:246-257
Timestamp: 2026-01-04T15:17:56.591Z
Learning: In Nano Press's secure token flow, private app tokens are registered via the `register_custom_app()` API before `create_and_build_custom_image()` is called. The `custom_apps` payload passed to `create_and_build_custom_image()` does NOT contain tokens (only name, githubUrl, branch, app_reference_id). Apps with tokens already exist in the database when `create_and_build_custom_image()` runs, so the fallback doc creation (when `frappe.db.exists("Apps", app_name)` is False) only applies to public apps and correctly sets `is_public=1`.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (4)
nano_press/templates/components/core/summary.html (4)
166-177: LGTM!The URL parameter parsing for deployment resume is well-structured with proper conditional checks before calling the appropriate resume handler.
591-599: Verify status-to-step mapping alignment with deployment flow.Comparing the step labels in the UI with the status mapping:
- Step 3 = "Creating site" → but
'Ready To Deploy' -> 3implies site creation is complete- Step 4 = "Preparing deployment" → but
'Deploying' -> 4- Step 5 = "Deploying site" → but
'Deployed' -> 5The mapping may be off by one. Based on
startDeployment(),'Ready To Deploy'status occurs after site creation (step 3 complete), so it should map to step 4.🔎 Suggested fix if mapping is confirmed incorrect
getStepFromStatus(status) { const statusMap = { - 'Not Deployed': 2, - 'Ready To Deploy': 3, - 'Deploying': 4, + 'Not Deployed': 3, + 'Ready To Deploy': 4, + 'Deploying': 5, 'Deployed': 5 }; return statusMap[status] || 1; }
13-21: LGTM!The conditional domain rendering with "Not configured" fallback provides clear user feedback and handles edge cases properly with the
.trim()check.
225-226: URL state management approach is sound.The
replaceStatecalls enable deployment resume across page refreshes. Note that the/self-hostpath is hardcoded in multiple places — if the route ever changes, these would need updating. Consider extracting to a constant if this becomes a maintenance concern.Also applies to: 261-262, 514-514, 536-536
| "fieldtype": "Link", | ||
| "label": "Custom Image", | ||
| "link_filters": "[[\"Custom Image\",\"build_status\",\"=\",\"Built\"]]", | ||
| "link_filters": "[[\"Custom Image\",\"build_status\",\"in\",[\"Built\",\"Building\"]]]", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for deployment/provisioning logic related to custom_image status
echo "=== Searching for frappe_site.py file ==="
find . -name "frappe_site.py" -type f
echo -e "\n=== Searching for custom_image related code in frappe_site.py ==="
rg -n "custom_image" nano_press/nano_press/doctype/frappe_site/ --type=py -A 3 -B 3
echo -e "\n=== Searching for build_status checks in deployment code ==="
rg -n "build_status" . --type=py -A 2 -B 2
echo -e "\n=== Looking for deployment/provisioning methods ==="
rg -n "def.*deploy|def.*provision|def.*resume" nano_press/nano_press/doctype/frappe_site/ --type=py -A 5
echo -e "\n=== Checking for frappe.throw related to custom image or build status ==="
rg -n "frappe\.(throw|msgprint)" nano_press/nano_press/doctype/frappe_site/ --type=py -A 2 -B 1Repository: BuildWithHussain/nano_press
Length of output: 11804
🏁 Script executed:
#!/bin/bash
# Check if there's any validation in prepare_for_deployment or deploy_site that checks custom_image build status
echo "=== Checking prepare_for_deployment method ==="
sed -n '141,175p' nano_press/nano_press/doctype/frappe_site/frappe_site.py
echo -e "\n=== Checking deploy_site method ==="
sed -n '175,210p' nano_press/nano_press/doctype/frappe_site/frappe_site.py
echo -e "\n=== Checking get_deployment_vars for any custom_image status validation ==="
sed -n '103,140p' nano_press/nano_press/doctype/frappe_site/frappe_site.py
echo -e "\n=== Checking get_docker_image more closely ==="
sed -n '92,102p' nano_press/nano_press/doctype/frappe_site/frappe_site.pyRepository: BuildWithHussain/nano_press
Length of output: 4157
Add validation to prevent deployment with incomplete custom images.
The link filter change allows selecting Custom Images with "Building" status, but the deployment code does not validate that the image is actually built. The get_docker_image() method (line 92-99) returns image_tag directly without checking build_status, and image_tag is set when the build starts, not when it completes. This allows deployments to proceed with incomplete Docker images, causing failures.
Add a check in get_docker_image() to verify build_status == "Built" before returning the image tag, or raise an error if the custom image build is still in progress or failed:
if custom_img.image_tag and custom_img.build_status == "Built":
return custom_img.image_tag
🤖 Prompt for AI Agents
In @nano_press/nano_press/doctype/frappe_site/frappe_site.json at line 104,
get_docker_image() currently returns custom_img.image_tag without validating
build_status, allowing deployments to use images that are still "Building" or
failed; update get_docker_image() to check custom_img.build_status and only
return custom_img.image_tag when build_status == "Built", otherwise raise a
user-facing error (or abort) indicating the custom image build is not complete
or failed so deployments cannot proceed with incomplete images.
… deployment tracking This update enables automatic status synchronization between Custom Image builds and Frappe Sites, allowing the frontend to track the entire deployment lifecycle by polling a single DocType.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @nano_press/nano_press/doctype/custom_image/custom_image.py:
- Around line 115-118: The manual commits flagged by semgrep are missing the
required inline justification; update both occurrences of frappe.db.commit() in
this file (the one inside Custom Image._update_status and the other
frappe.db.commit call later in the file) to keep the existing "nosemgrep:
frappe-semgrep-rules.rules.frappe-manual-commit" annotation and add a brief
same-line comment explaining why the manual commit is necessary (e.g., to ensure
the build_status change is persisted immediately before calling
_sync_status_to_frappe_sites or before subsequent cross-site operations), so the
comment sits on the same line as frappe.db.commit() and references the reason
for immediate DB persistence.
In @nano_press/nano_press/doctype/frappe_site/frappe_site.py:
- Around line 103-117: The validate_custom_image method currently only warns on
failed builds using frappe.msgprint, allowing submission to proceed; change the
behavior to block invalid state by replacing the msgprint with frappe.throw when
custom_image.build_status == "Failed" (in validate_custom_image, referencing
self.custom_image and the fetched Custom Image doc) so the document cannot be
saved/deployed with a failed image; if the warning-only behavior is intentional
instead, add a clear comment above validate_custom_image explaining why failed
images are permitted and update any tests or callers expecting a hard failure
accordingly.
- Around line 355-371: rebuild_custom_image_for_site sets the Frappe Site status
to "Building" and manually commits before enqueueing the build, which can lead
to inconsistent state; remove the redundant frappe.db.set_value and
frappe.db.commit calls in rebuild_custom_image_for_site and rely on
custom_image.enqueue_build_custom_image() (which already calls
_update_status("Building") and _sync_status_to_frappe_sites()) or move the
status update into the enqueue/build workflow or a post-commit hook so the
"Building" status is only set after the transaction and enqueue succeed.
In @nano_press/public/css/main.css:
- Around line 10-21: The blinking animation defined by the utility animate-blink
and the @keyframes blink doesn’t respect user motion preferences; wrap or
override that animation inside a prefers-reduced-motion: reduce media query so
users who prefer reduced motion don’t get the infinite blink. Specifically, add
a prefers-reduced-motion rule that sets animation: none (or animation-duration:
0s / animation-iteration-count: 1) for .animate-blink (or the @utility
animate-blink) and ensure the @keyframes blink remains unchanged for users
without the preference.
In @nano_press/templates/components/core/summary.html:
- Around line 178-189: When init() detects resuming via URL params (isComplete
or isDeploying) ensure the Alpine store's installer.currentStep is set so the
summary view becomes visible; before calling resumeCompletedDeployment(siteName)
or resumeDeployment(siteName) set $store.installer.currentStep = 4 (or set it
inside those functions) so x-show="$store.installer.currentStep === 4" will
evaluate true; update either resumeCompletedDeployment() and resumeDeployment()
to explicitly set $store.installer.currentStep = 4 on resume or add that
assignment in init() right before invoking them.
- Around line 515-625: The resumeDeployment method creates a local pollInterval
that isn't tracked on the component and so never cleared on destroy; add a data
property pollIntervalId (e.g., pollIntervalId: null), assign this.pollIntervalId
= setInterval(...) inside resumeDeployment, replace any
clearInterval(pollInterval) calls with clearInterval(this.pollIntervalId) and
set this.pollIntervalId = null afterward, and implement a lifecycle cleanup
(beforeDestroy or unmounted, or a destroy method) that clears
this.pollIntervalId if set to prevent leaks and stray network calls.
🧹 Nitpick comments (1)
nano_press/nano_press/doctype/frappe_site/frappe_site.py (1)
183-189: Localize error message for consistency.The error message at line 188 is not wrapped with
_()for localization, unlike other error messages in this file (lines 95, 97, 100, 111-112). For consistency and i18n support, localize this message as well.♻️ Proposed fix
if self.is_custom and self.custom_image: custom_image = frappe.get_cached_doc("Custom Image", self.custom_image) if custom_image.build_status != "Built": return { "status": 400, - "message": f"Custom Image is not ready (status: {custom_image.build_status}). Please wait for build to complete.", + "message": _("Custom Image is not ready (status: {0}). Please wait for build to complete.").format( + custom_image.build_status + ), }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
nano_press/nano_press/doctype/custom_image/custom_image.pynano_press/nano_press/doctype/frappe_site/frappe_site.jsonnano_press/nano_press/doctype/frappe_site/frappe_site.pynano_press/public/css/main.cssnano_press/templates/components/core/summary.html
🚧 Files skipped from review as they are similar to previous changes (1)
- nano_press/nano_press/doctype/frappe_site/frappe_site.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Venkateshvenki404224
Repo: BuildWithHussain/nano_press PR: 22
File: nano_press/nano_press/doctype/custom_image/custom_image.py:246-257
Timestamp: 2026-01-04T15:17:56.591Z
Learning: In Nano Press's secure token flow, private app tokens are registered via the `register_custom_app()` API before `create_and_build_custom_image()` is called. The `custom_apps` payload passed to `create_and_build_custom_image()` does NOT contain tokens (only name, githubUrl, branch, app_reference_id). Apps with tokens already exist in the database when `create_and_build_custom_image()` runs, so the fallback doc creation (when `frappe.db.exists("Apps", app_name)` is False) only applies to public apps and correctly sets `is_public=1`.
📚 Learning: 2026-01-04T15:17:56.591Z
Learnt from: Venkateshvenki404224
Repo: BuildWithHussain/nano_press PR: 22
File: nano_press/nano_press/doctype/custom_image/custom_image.py:246-257
Timestamp: 2026-01-04T15:17:56.591Z
Learning: In Nano Press's secure token flow, private app tokens are registered via the `register_custom_app()` API before `create_and_build_custom_image()` is called. The `custom_apps` payload passed to `create_and_build_custom_image()` does NOT contain tokens (only name, githubUrl, branch, app_reference_id). Apps with tokens already exist in the database when `create_and_build_custom_image()` runs, so the fallback doc creation (when `frappe.db.exists("Apps", app_name)` is False) only applies to public apps and correctly sets `is_public=1`.
Applied to files:
nano_press/nano_press/doctype/custom_image/custom_image.py
🧬 Code graph analysis (1)
nano_press/nano_press/doctype/frappe_site/frappe_site.py (1)
nano_press/nano_press/doctype/custom_image/custom_image.py (1)
enqueue_build_custom_image(244-255)
🪛 Ruff (0.14.10)
nano_press/nano_press/doctype/custom_image/custom_image.py
168-168: Do not catch blind exception: Exception
(BLE001)
202-202: Do not catch blind exception: Exception
(BLE001)
267-267: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (10)
nano_press/nano_press/doctype/frappe_site/frappe_site.py (2)
92-101: LGTM: Server validation messages properly localized.The error messages in
validate_server()are now correctly wrapped with_()for internationalization and use.format()for safe string interpolation. This follows Frappe framework best practices.
57-79: Fix inconsistent validation and message visibility for Custom Image status.The
validate_custom_image()method only checks for "Failed" status but does not validate or block "Draft" status, while_check_custom_image_status()defensively handles Draft withfrappe.msgprint(). This is inconsistent and ineffective: Draft status cannot be selected via the UI (link_filters restrict to "Built" or "Building"), but if Draft somehow reaches this code, the msgprint won't be visible in non-interactive contexts (API calls, background jobs).Additionally, the manual commits at lines 67 and 72 lack justification comments despite the nosemgrep suppressions.
Recommendations:
- Add validation in
validate_custom_image()to throw an error for Draft status, preventing site creation entirely instead of relying on post-insert msgprint- Replace the Draft msgprint with a
frappe.throw()that interrupts creation before the database is modified- Add comment explaining why manual commits are necessary if they must remain (e.g., "Status must persist immediately to reflect build state in external systems")
nano_press/templates/components/core/summary.html (2)
441-462: LGTM: Rebuild flow properly implemented.The
rebuildCustomImage()function correctly calls the backend API, handles success/error responses with appropriate user feedback via toasts, and resumes deployment polling. The error handling is defensive and user-friendly.
464-493: LGTM: Simplified build function with clearer separation of concerns.The
buildCustomImage()function now focuses solely on initiating the build and returning the image name, delegating polling and status checks to theresumeDeployment()function. This improves code clarity and reduces duplication.nano_press/nano_press/doctype/custom_image/custom_image.py (6)
19-29: LGTM: Comprehensive Docker tag sanitization.The
_set_image_tag()method now properly sanitizes image names by:
- Removing invalid characters (only
a-z0-9_.-allowed)- Collapsing repeated separators
- Stripping leading
.and-characters- Providing a safe fallback ("image") when sanitization results in an empty string
This addresses the Docker image tag validation requirements and prevents build failures from invalid tag names.
63-84: LGTM: App sorting correctly handles sparse configurations.The
_sort_apps_by_order()method now properly handles cases where someapps_configentries may lack anapp_nameby building an explicit order mapping and sorting by app names rather than positional indices. This prevents misalignment issues and ensures apps are ordered correctly.
86-91: LGTM: Reusing computed base64 value avoids redundancy.The method now uses
self.apps_json_base64(computed inbefore_save) instead of regenerating it, which is more efficient and ensures consistency between what's saved and what's deployed.
205-241: LGTM: Status synchronization properly implemented.The
_sync_status_to_frappe_sites()method correctly:
- Checks if status actually changed before querying
- Filters sites that need updating
- Maps build statuses to appropriate site statuses
- Logs synchronization for debugging
This ensures dependent Frappe Sites stay in sync with their Custom Image build lifecycle.
(The manual commit at line 232 is addressed in a separate comment.)
271-332: LGTM: Proper validation and secure token flow implemented.The
create_and_build_custom_image()function now includes:
- Server existence check (line 276) before attempting operations
- Server status validation (lines 280-283) ensuring it's prepared
- Required field validation for custom apps (lines 304-307)
- Clear error messages with context
The
is_public=1setting at line 317 is correct per the secure token flow: private apps with tokens are pre-registered viaregister_custom_app()API, so this fallback only executes for public apps.Based on learnings, the token handling architecture is correctly implemented.
257-268: LGTM: Clean API interfaces for preview and status.Both
preview_apps_json_for_form()andget_build_status()provide clean, well-structured API responses with appropriate error handling. The preview method safely returns errors without breaking the UI, and the status method provides all necessary build information.Also applies to: 335-343
| def _build_repo_url(self, app_doc): | ||
| repo_url = (app_doc.repo_url or "").strip() | ||
|
|
||
| return json.dumps(sorted_apps, indent=2) | ||
| if not app_doc.is_public and app_doc.pat_token and repo_url.startswith("https://"): | ||
| url_parts = repo_url.replace("https://", "").split("/", 1) | ||
| if len(url_parts) == 2: | ||
| return f"https://{app_doc.pat_token}@{url_parts[0]}/{url_parts[1]}" | ||
|
|
||
| def set_image_tag(self) -> str: | ||
| """Generate image tag using just the image name.""" | ||
| clean_name = self.image_name.lower().replace(" ", "-") | ||
| self.image_tag = f"{clean_name}:latest" | ||
| return repo_url |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the specific lines
if [ -f "nano_press/nano_press/doctype/custom_image/custom_image.py" ]; then
echo "=== File exists. Checking lines 53-61 ==="
sed -n '50,65p' nano_press/nano_press/doctype/custom_image/custom_image.py | cat -n
else
echo "File not found at expected path. Searching for it..."
find . -name "custom_image.py" -type f 2>/dev/null
fiRepository: BuildWithHussain/nano_press
Length of output: 767
🏁 Script executed:
# Check if commit f048dd1 exists in the repository
git log --oneline | head -20
echo "---"
git show f048dd1 --stat 2>/dev/null || echo "Commit f048dd1 not found in history"Repository: BuildWithHussain/nano_press
Length of output: 1170
🏁 Script executed:
# Search for any references to this security issue or the _build_repo_url method
rg "_build_repo_url" --type py -A 10 -B 2Repository: BuildWithHussain/nano_press
Length of output: 2317
🏁 Script executed:
# Check git history for changes to custom_image.py
git log --oneline nano_press/nano_press/doctype/custom_image/custom_image.py 2>/dev/null | head -20Repository: BuildWithHussain/nano_press
Length of output: 168
🏁 Script executed:
# Show the actual diff for the custom_image.py changes in commit f048dd1
git show f048dd1 -- nano_press/nano_press/doctype/custom_image/custom_image.pyRepository: BuildWithHussain/nano_press
Length of output: 1775
🏁 Script executed:
# Check when _build_repo_url is called and how the URL is used
rg "_build_repo_url|_get_docker_build_config" --type py -A 5 -B 2Repository: BuildWithHussain/nano_press
Length of output: 1482
🏁 Script executed:
# Look for any environment variable or secure credential passing mechanisms
rg "pat_token|PAT|token" nano_press/nano_press/doctype/custom_image/custom_image.py -B 3 -A 3Repository: BuildWithHussain/nano_press
Length of output: 421
🏁 Script executed:
# Check if there are any existing Custom Apps with is_public=0 that would be vulnerable
rg "is_public.*0|is_public.*False" nano_press/nano_press/doctype/ --type py -B 3 -A 3Repository: BuildWithHussain/nano_press
Length of output: 447
🏁 Script executed:
# Look for any tests or documentation about the token handling
rg "pat_token|_build_repo_url" nano_press/ --type py --type json --type md -B 2 -A 2 | head -50Repository: BuildWithHussain/nano_press
Length of output: 3219
Security: PAT tokens embedded in URLs for all private custom apps.
The _build_repo_url() method injects PAT tokens directly into repository URLs (line 59), exposing them to logs, process listings, and error messages. Critically, the validate() method in apps.py automatically forces is_public = 0 whenever a custom app has a pat_token, which systematically triggers the vulnerable code path. This means tokens are embedded in URLs for every private custom app by design.
The claim that this was "Addressed in commit f048dd1" is incorrect—that commit only changed the function signature in create_and_build_custom_image() but did not fix the underlying vulnerability.
Tokens can leak in:
- Ansible playbook logs and build output
- Git command process listings (
ps aux) - Error messages and stack traces
- System logs and monitoring tools
Replace URL-embedded authentication with secure alternatives: pass tokens as separate deployment variables, use Git credential helpers, or store them as environment variables in the Ansible playbook.
| def _update_status(self, status): | ||
| frappe.db.set_value("Custom Image", self.name, "build_status", status, update_modified=False) | ||
| frappe.db.commit() # nosemgrep: frappe-semgrep-rules.rules.frappe-manual-commit | ||
| self._sync_status_to_frappe_sites() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add inline justification for manual commits as required by linter.
The manual commits at lines 117 and 232 have nosemgrep comments but lack the inline justification that the linter requires. Per the semgrep rule and previous review comments, manual commits should include a comment on the same line explaining why they're necessary.
♻️ Proposed fix
def _update_status(self, status):
frappe.db.set_value("Custom Image", self.name, "build_status", status, update_modified=False)
- frappe.db.commit() # nosemgrep: frappe-semgrep-rules.rules.frappe-manual-commit
+ frappe.db.commit() # Manual commit: status must be visible to UI immediately during background build # nosemgrep: frappe-semgrep-rules.rules.frappe-manual-commit
self._sync_status_to_frappe_sites() for site in linked_sites:
if new_status != site.status:
frappe.db.set_value("Frappe Site", site.name, "status", new_status, update_modified=False)
frappe.logger().info(
f"Synced status for Frappe Site {site.name}: {site.status} → {new_status}"
)
- frappe.db.commit() # nosemgrep: frappe-semgrep-rules.rules.frappe-manual-commit
+ frappe.db.commit() # Manual commit: sync status to sites during background build # nosemgrep: frappe-semgrep-rules.rules.frappe-manual-commitAlso applies to: 232-232
🤖 Prompt for AI Agents
In @nano_press/nano_press/doctype/custom_image/custom_image.py around lines 115
- 118, The manual commits flagged by semgrep are missing the required inline
justification; update both occurrences of frappe.db.commit() in this file (the
one inside Custom Image._update_status and the other frappe.db.commit call later
in the file) to keep the existing "nosemgrep:
frappe-semgrep-rules.rules.frappe-manual-commit" annotation and add a brief
same-line comment explaining why the manual commit is necessary (e.g., to ensure
the build_status change is persisted immediately before calling
_sync_status_to_frappe_sites or before subsequent cross-site operations), so the
comment sits on the same line as frappe.db.commit() and references the reason
for immediate DB persistence.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @nano_press/templates/components/core/server.html:
- Line 164: The template sets userServers via "user_servers | tojson" which can
render null and later cause a runtime error when calling
this.userServers.find(); update the userServers initialization to ensure a safe
empty-array fallback — either apply the Jinja default filter (e.g.,
default('[]')) to user_servers before tojson or append a JS fallback (e.g., ||
[]) after the tojson output so that userServers is always an array for
subsequent calls to this.userServers.find().
🧹 Nitpick comments (1)
nano_press/templates/components/core/server.html (1)
271-321: Consolidate duplicate toast notifications.The current flow shows multiple success toasts for a single operation:
ping_servershows "Server connection verified successfully" (line 313)create_server_docshows "Server record created successfully" (line 285)nextStepshows "Server verified and created successfully" (line 248-250)This results in three success toasts for one user action, which can be confusing. Consider removing the toast notifications from
ping_serverandcreate_server_doc, keeping only the consolidated message innextStep.♻️ Proposed refactor
async function create_server_doc(server_ip, ssh_user, ssh_port) { const url = '/api/resource/Server'; try { const payload = { server_ip, ssh_user, ssh_port, traefik_email: '{{ user_email }}' }; const response = await frappe_call(url, payload, "POST"); if (response?.data?.name) { - const message = ("Server record created successfully"); - toast(message, { type: "success" }); return response.data; } else { throw new Error("Failed to create server record."); } } catch (err) { const message = err.message || ("Error creating server record"); toast(message, { type: "danger" }); throw err; } } async function ping_server(server_ip, ssh_user = "root", ssh_port = 22) { const url = '/api/v2/method/nano_press.api.ping_server'; try { const payload = { host: server_ip, user: ssh_user, port: ssh_port }; const response = await frappe_call(url, payload, "POST"); if (!response?.data?.ok) { throw new Error(response?.data?.message || "Server is unreachable."); } - const message = ("Server connection verified successfully"); - toast(message, { type: "success" }); - return response.data; } catch (err) { const message = err.message || ("Failed to verify server connection"); toast(message, { type: "danger" }); throw err; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nano_press/templates/components/core/server.html
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Venkateshvenki404224
Repo: BuildWithHussain/nano_press PR: 22
File: nano_press/nano_press/doctype/custom_image/custom_image.py:246-257
Timestamp: 2026-01-04T15:17:56.591Z
Learning: In Nano Press's secure token flow, private app tokens are registered via the `register_custom_app()` API before `create_and_build_custom_image()` is called. The `custom_apps` payload passed to `create_and_build_custom_image()` does NOT contain tokens (only name, githubUrl, branch, app_reference_id). Apps with tokens already exist in the database when `create_and_build_custom_image()` runs, so the fallback doc creation (when `frappe.db.exists("Apps", app_name)` is False) only applies to public apps and correctly sets `is_public=1`.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (5)
nano_press/templates/components/core/server.html (5)
14-59: LGTM! Server selection UI integrates cleanly.The conditional layout and server selection dropdown are well-implemented. The manual IP input correctly resets
selectedServerNameand verification state, preventing inconsistencies between selected and manually entered servers.
166-189: LGTM! Server selection logic is well-structured.The method correctly populates form fields from the selected server, handles verification state appropriately, and provides user feedback via toast notifications. The String conversion for
ssh_portat line 177 ensures compatibility with the input field's text model.
196-224: LGTM! Data clearing is comprehensive and user-friendly.The confirmation dialog, comprehensive localStorage key list, and delayed reload provide a good user experience. The toast notification gives clear feedback before the page reloads.
226-267: LGTM! Step navigation logic is correct.The function properly handles both verified and unverified states, validates IP addresses, and stores the server name from the API response at line 254. Error handling and loading state management are appropriate.
323-327: LGTM! Clean composition of verification steps.The function appropriately sequences the ping and create operations, with error propagation handled by the underlying functions.
Cleaned up several code quality issues identified during review to improve reliability and user experience. Fixes: - Remove duplicate status sync call in custom image build failure handler - Fix polling interval memory leak by tracking and cleaning up on component destroy - Ensure summary component visible when resuming from URL parameters - Add null safety fallback for user servers array to prevent runtime errors - Remove redundant status update in rebuild endpoint (already handled by sync) - Upgrade failed image validation from warning to hard block for data integrity - Add accessibility support for users with reduced motion preferences
Now the Summery Page is refresh proof ! @Rl0007 |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.