Repository navigation
Conversation
 [Medium impact] The PR reaches dashboard setup, site creation, and backup workflows on a bench. Resolve the storage picker, site-job tracking, and repository requirements before merging. Reviews (2) · Last reviewed commit: "feat(ui): onboarding github via frappe c..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <Select | ||
| v-model="storage.region" | ||
| label="Region" | ||
| :options="storage.regionOptions" | ||
| :disabled="storage.loadingRegions" | ||
| /> |
There was a problem hiding this comment.
The storage step has no provider picker. On a new bench, useObjectStorage silently chooses AWS, so DigitalOcean and Hetzner users cannot select their regions. Leaving the advertised optional endpoint blank then sends backups to AWS. Add the provider picker used in settings.
Prompt To Fix With AI
This is a comment left during a code review.
Path: admin/frontend/dashboard/src/components/navigation/SetupChecklist.vue
Line: 259-264
Comment:
**Storage provider cannot be changed**
The storage step has no provider picker. On a new bench, `useObjectStorage` silently chooses AWS, so DigitalOcean and Hetzner users cannot select their regions. Leaving the advertised optional endpoint blank then sends backups to AWS. Add the provider picker used in settings.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const taskId = await site.create() | ||
|
|
||
| if (!taskId) return false | ||
|
|
||
| toast.success(`Creating ${name}. You can keep setting up meanwhile.`) | ||
| await loadSites() | ||
|
|
||
| return true |
There was a problem hiding this comment.
createSite drops taskId and reloads sites only once, immediately after the job is queued. If the job has not written the site yet, “First site” stays incomplete after creation finishes; failures also never reach this dialog. Keep the task ID, watch the result, and reload sites when it finishes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: admin/frontend/dashboard/src/components/navigation/SetupChecklist.vue
Line: 146-153
Comment:
**Site progress stops updating**
`createSite` drops `taskId` and reloads sites only once, immediately after the job is queued. If the job has not written the site yet, “First site” stays incomplete after creation finishes; failures also never reach this dialog. Keep the task ID, watch the result, and reload sites when it finishes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| const submit = async () => { | ||
| const step = currentStep.value | ||
|
|
||
| if (step.done || (await step.save())) advance() |
There was a problem hiding this comment.
The new dialog has no frontend tests for opening, minimizing, dismissing, or advancing its steps. The added tests cover stored settings, not the dialog. The repository requires tests for behavior changes, so add this coverage before merging, including failed saves and delayed site creation.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: admin/frontend/dashboard/src/components/navigation/SetupChecklist.vue
Line: 138-141
Comment:
**Checklist behavior lacks tests**
The new dialog has no frontend tests for opening, minimizing, dismissing, or advancing its steps. The added tests cover stored settings, not the dialog. The repository requires tests for behavior changes, so add this coverage before merging, including failed saves and delayed site creation.
**Context Used:** CLAUDE.md ([source](https://github.com/frappe/pilot/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@siduck also please wire up the central <-> github connection, that has been merged and working. In cloud, that will be the most used option instead of pat token. To simulate, you can ask llm to simulate the metadata service to pilot with your local central site. |
will do |
| <template v-else-if="currentStep.id === 'github' && github.viaCentral"> | ||
| <p class="mb-4 text-ink-gray-7 text-p-base"> | ||
| Connected through Frappe Cloud. Pilot can read these repos. |
There was a problem hiding this comment.
The added GitHub panel pushes SetupChecklist.vue to 544 lines. The repository requires splitting files that grow past roughly 500 lines. Move the GitHub or site form into a separate component before merging.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: admin/frontend/dashboard/src/components/navigation/SetupChecklist.vue
Line: 308-310
Comment:
**Onboarding component is too large**
The added GitHub panel pushes `SetupChecklist.vue` to 544 lines. The repository requires splitting files that grow past roughly 500 lines. Move the GitHub or site form into a separate component before merging.
**Context Used:** CLAUDE.md ([source](https://github.com/frappe/pilot/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
simplescreenrecorder-2026-10-17_17.46.36.mp4