Add download and deployment#999
Open
Amerlander wants to merge 6 commits into
Open
Conversation
…ent, and site download
…d publishing functionality
📝 WalkthroughWalkthroughAdds backend endpoints to export site ZIPs, check deploy status, and deploy sites via Wrangler; a migration to store Cloudflare credentials; frontend Cloudflare settings and deployment UI; dev env and dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Browser
participant Server as PocketBase API
participant FileSystem as PocketBase Filesystem
participant ZIP as ZIP Writer
User->>Browser: Click "Download Site"
Browser->>Server: GET /api/palacms/site-zip/{siteId}
Server->>Server: Validate access & resolve host
Server->>FileSystem: Walk sites/{host}/ directory
FileSystem-->>Server: File list
loop For each file
Server->>FileSystem: Read file content
FileSystem-->>Server: File bytes
Server->>ZIP: Add entry to ZIP
end
ZIP-->>Server: ZIP buffer
Server-->>Browser: Respond application/zip (attachment)
Browser->>User: Save file
sequenceDiagram
actor User
participant Browser as Browser
participant Server as PocketBase API
participant Wrangler as Wrangler CLI
participant Cloudflare as Cloudflare Pages
User->>Browser: Click "Deploy"
Browser->>Server: POST /api/palacms/deploy/{siteId} (branch?)
Server->>Server: Validate site, credentials, siteDir
Server->>Wrangler: Execute wrangler deploy (env vars for CF creds)
Wrangler->>Cloudflare: Push site files
Cloudflare-->>Wrangler: Deployment response (status, url)
Wrangler-->>Server: Return status & URL
Server-->>Browser: JSON {status, url}
Browser->>User: Show deploy result/toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
src/routes/dashboard/sites/+page.svelte (1)
54-61:⚠️ Potential issue | 🟡 Minor
deleting_groupcan stay stuck on early return.
deleting_groupis set totruebefore the null check. Ifactive_site_groupis missing, the function returns without resetting state.Proposed fix
async function handle_group_delete() { - deleting_group = true - if (!active_site_group) return - SiteGroups.delete(active_site_group.id) - await self.commit() - deleting_group = false - is_delete_group_open = false + if (!active_site_group) return + deleting_group = true + try { + SiteGroups.delete(active_site_group.id) + await self.commit() + is_delete_group_open = false + } finally { + deleting_group = false + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/dashboard/sites/`+page.svelte around lines 54 - 61, handle_group_delete sets deleting_group = true before checking active_site_group, so if active_site_group is falsy the function returns with deleting_group stuck true; update handle_group_delete to either perform the null check first (only set deleting_group = true after confirming active_site_group exists) or wrap the deletion logic in a try/finally that resets deleting_group = false and is_delete_group_open = false in the finally block; reference the handle_group_delete function, the deleting_group and is_delete_group_open flags, and the SiteGroups.delete/ self.commit calls when making the change.
🧹 Nitpick comments (2)
src/routes/dashboard/sites/+page.svelte (1)
63-84: Avoid request functions depending on mutablecurrent_site.
download_site_file()anddeploy_site()read sharedcurrent_site, which is mutable across menu interactions. Pass the selectedsiteas a parameter to make each action deterministic.Also applies to: 119-139, 301-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/dashboard/sites/`+page.svelte around lines 63 - 84, The functions download_site_file and deploy_site currently read the mutable shared variable current_site causing nondeterministic behavior; change their signatures to accept a site parameter (e.g., download_site_file(site) and deploy_site(site)), replace all uses of current_site inside those functions with the passed site (use site.id and site.host), and update every call site (menu buttons / event handlers referenced around the blocks at lines ~63-84, ~119-139, ~301-324) to pass the selected site object so each action operates on the intended site instance.src/lib/builder/views/editor/Toolbar.svelte (1)
52-75: Extract deployment API call into a shared helper.
handle_deployduplicates the deployment request flow also implemented insrc/routes/dashboard/sites/+page.svelte. Centralizing this avoids drift in auth/error/toast behavior.Also applies to: 396-401
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/builder/views/editor/Toolbar.svelte` around lines 52 - 75, handle_deploy duplicates the deployment request logic used elsewhere (e.g. in +page.svelte); extract the API call into a shared helper (e.g. export async function deploySite(instance, site) in a common module) that performs the POST to /api/palacms/deploy/:id, sets the Authorization header from instance.authStore.token, throws on non-ok and returns parsed JSON; replace the inline fetch in Toolbar.svelte's handle_deploy (and the other occurrence) to call deploySite, keep UI concerns (deploy_in_progress, toast.success/toast.error, console.error) in the callers, and ensure both callers import and use the single helper so auth/error/toast behavior stays consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/export.go`:
- Around line 143-144: The code currently returns raw command output (variable
out from CombinedOutput) to clients via e.InternalServerError, which can leak
sensitive details; instead, log the full output and error server-side (e.g.,
using the existing logger in internal/export.go) and change the
e.InternalServerError call in the cloudflare deployment failure path to return a
generic message like "cloudflare deployment failed" without appending
string(out). Keep the original err when logging but do not include out or other
execution details in the error returned to the caller.
- Around line 132-133: site.GetString("host") is used directly in filepath.Join
to build siteDir (filepath.Join(pb.DataDir(), "storage", "sites",
site.GetString("host"))) which permits path traversal; before joining, validate
and sanitize the host value (e.g. reject if empty, contains path separators or
"..", or is absolute) or normalize it to a safe token (use filepath.Clean and
ensure it does not start with ".." or os.PathSeparator, or restrict to a
whitelist/regex like /^[A-Za-z0-9.-]+$/ and/or use path.Base to strip directory
components) and only then use the sanitizedHost in filepath.Join to construct
siteDir.
- Around line 134-142: The deploy invocation currently uses exec.Command("npx",
"-y", "wrangler@latest", ...) and CombinedOutput() with no timeout; fix by
pinning the wrangler version and adding a command timeout: replace
"wrangler@latest" with the pinned version used in package.json (e.g.
"wrangler@^4.69.0" or an exact "wrangler@4.69.0") and create the process with a
context timeout (use context.WithTimeout and exec.CommandContext) around the
creation of cmd, defer cancel(), then call cmd.CombinedOutput() (or
cmd.Output()) and handle context deadline errors appropriately so hung deploys
are killed and the error reflects timeout.
In `@migrations/1766436666_add_cf_fields.go`:
- Around line 26-30: The migration adds a plain-text site field cfApiToken
(core.TextField with Name "cfApiToken") which stores a long-lived Cloudflare API
token unencrypted; remove this field from the site schema and instead persist
the token in a server-side secret store or an encrypted-at-rest secrets table
with strict access controls, or store only a non-sensitive site identifier and
fetch the token at runtime from a secure service; update the migration that
creates the core.TextField entry for "cfApiToken" to instead create or reference
the secure secret mechanism and add notes in the code where the token is
retrieved so callers use the secret store API rather than reading a site field.
In `@src/lib/common/models/Site.ts`:
- Around line 12-14: The Site zod schema currently exposes cfApiToken
(cfApiToken) which leaks credentials to clients; remove cfApiToken from the
shared Site schema definition and any exports so tokens are never serialized to
client-facing models, and instead create a separate backend-only input DTO or
endpoint that accepts cfApiToken for write operations (e.g. a Cloudflare token
handler) so token handling remains write-only on the server; also update any
usages of Site (e.g. deserialization, type exports) to stop reading or passing
cfApiToken to client code and add server-side validation for the new
backend-only DTO.
In `@src/routes/dashboard/sites/`+page.svelte:
- Around line 110-112: The form only assigns data.cfApiToken when cf_token is
truthy, so clearing the token (empty string or falsy value) doesn't persist and
the old secret remains; remove the truthy guard and always set data.cfApiToken =
cf_token (or explicitly set to ''/null when cf_token is empty) wherever the
current block assigns cfApiToken so an intentionally cleared token is written
back.
---
Outside diff comments:
In `@src/routes/dashboard/sites/`+page.svelte:
- Around line 54-61: handle_group_delete sets deleting_group = true before
checking active_site_group, so if active_site_group is falsy the function
returns with deleting_group stuck true; update handle_group_delete to either
perform the null check first (only set deleting_group = true after confirming
active_site_group exists) or wrap the deletion logic in a try/finally that
resets deleting_group = false and is_delete_group_open = false in the finally
block; reference the handle_group_delete function, the deleting_group and
is_delete_group_open flags, and the SiteGroups.delete/ self.commit calls when
making the change.
---
Nitpick comments:
In `@src/lib/builder/views/editor/Toolbar.svelte`:
- Around line 52-75: handle_deploy duplicates the deployment request logic used
elsewhere (e.g. in +page.svelte); extract the API call into a shared helper
(e.g. export async function deploySite(instance, site) in a common module) that
performs the POST to /api/palacms/deploy/:id, sets the Authorization header from
instance.authStore.token, throws on non-ok and returns parsed JSON; replace the
inline fetch in Toolbar.svelte's handle_deploy (and the other occurrence) to
call deploySite, keep UI concerns (deploy_in_progress,
toast.success/toast.error, console.error) in the callers, and ensure both
callers import and use the single helper so auth/error/toast behavior stays
consistent.
In `@src/routes/dashboard/sites/`+page.svelte:
- Around line 63-84: The functions download_site_file and deploy_site currently
read the mutable shared variable current_site causing nondeterministic behavior;
change their signatures to accept a site parameter (e.g.,
download_site_file(site) and deploy_site(site)), replace all uses of
current_site inside those functions with the passed site (use site.id and
site.host), and update every call site (menu buttons / event handlers referenced
around the blocks at lines ~63-84, ~119-139, ~301-324) to pass the selected site
object so each action operates on the intended site instance.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
internal/export.gomain.gomigrations/1766436666_add_cf_fields.gopackage.jsonsrc/lib/builder/views/editor/Toolbar.sveltesrc/lib/common/models/Site.tssrc/routes/dashboard/sites/+page.svelte
Comment on lines
+26
to
+30
| &core.TextField{ | ||
| Name: "cfApiToken", | ||
| Required: false, | ||
| Hidden: true, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root/repo && git ls-files | grep -E "(migrations|pocketbase)" | head -20Repository: palacms/palacms
Length of output: 119
🌐 Web query:
PocketBase core.TextField Hidden true encryption at rest
💡 Result:
In PocketBase, core.TextField{Hidden: true} does not provide encryption-at-rest. It only omits the field from the API response output (“Hidden hides the field from the API response”). [1][2]
PocketBase’s maintainer confirms there is no built-in encryption for database (record) content at rest; if you need it, you must implement it yourself (encrypt specific field values in your app) or use external tooling (e.g., encrypt the DB file / disk, SQLite encryption extensions, etc.). [3]
PocketBase does have an optional “settings encryption” feature (for app settings like SMTP password / S3 credentials), enabled via --encryptionEnv=YOUR_ENV_VAR with a 32-char key—but that is separate from record field storage and doesn’t make Hidden fields encrypted on disk. [4]
Sources
[1] PocketBase JSVM reference – core.TextField.hidden (“Hidden hides the field from the API response”) (pocketbase.io)
[2] Go package docs (github.com/pocketbase/pocketbase/core) – Hidden field comment (pkg.go.dev)
[3] PocketBase GitHub Discussion #1961 – “There is no built-in encryption for the db content” (github.com)
[4] PocketBase “Going to production” docs – “Enable settings encryption” with --encryptionEnv (pocketbase.io)
Citations:
- 1: https://pocketbase.io/jsvm/interfaces/core.TextField.html
- 2: https://pkg.go.dev/github.com/pocketbase/pocketbase/core
- 3: Database encryption pocketbase/pocketbase#1961
- 4: https://pocketbase.io/docs/going-to-production/?utm_source=openai
🌐 Web query:
PocketBase field encryption security hidden fields
💡 Result:
PocketBase “Hidden” fields: what they do (and don’t)
- “Hidden” is primarily a serialization/visibility control, meant to prevent “regular user” access to a field’s value in the public API output (and you can also hide/unhide dynamically at runtime via
record.Hide(...)/record.Unhide(...), typically inOnRecordEnrich). (pocketbase.io) - Hidden ≠ encrypted: the value is still stored in the database as usual; “Hidden” doesn’t provide cryptographic protection. (So if the DB file/backups leak, hidden fields leak too.)
Field/value encryption support in PocketBase
- PocketBase does not provide built-in encryption for database record content / field-level encryption. If you need encrypted fields, you must implement it yourself (e.g., encrypt before save, decrypt after read) in your app logic/hooks, and manage keys securely. (github.com)
- PocketBase does support encrypting PocketBase settings (where secrets like SMTP/S3 creds may live) via an env key +
--encryptionEnv=.... (pocketbase.io) - If you’re using PocketBase’s JS hooks/runtime, there’s a provided
encrypt(data, key)helper in the JSVM security API (you still own key management and the overall scheme). (pocketbase.io)
Security implications / best practice
- Treat API Rules (collection/list/view/create/update/delete rules) as your real access-control boundary; use Hidden to reduce accidental exposure in responses, not as a security control.
- For truly sensitive data, prefer (a) a separate collection with very strict rules, (b) storing only hashes/tokens when possible, or (c) app-managed encryption (with keys not stored alongside the DB).
Note on auth collections
- PocketBase auth collections include system fields like
password(bcrypt-hashed) andtokenKey, and those are marked Hidden by default. (deepwiki.com)
Citations:
- 1: https://pocketbase.io/docs/go-records?utm_source=openai
- 2: Database encryption pocketbase/pocketbase#1961
- 3: https://pocketbase.io/docs/going-to-production/?utm_source=openai
- 4: https://pocketbase.io/jsvm/functions/_security.encrypt.html?utm_source=openai
- 5: https://deepwiki.com/pocketbase/pocketbase/4.5-auth-collections?utm_source=openai
Do not store Cloudflare API tokens as plain-text site fields.
Hidden: true only prevents the field from appearing in API responses; it provides no encryption at rest. The token value remains stored in the database in plaintext and is exposed in any database dump or backup. This unnecessary blast radius for a long-lived credential can be eliminated.
Store API tokens in a separate server-side secret store or encrypted-at-rest secret table with strict access controls. Alternatively, consider storing only a non-sensitive site identifier and retrieving the actual token from a secure external service at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@migrations/1766436666_add_cf_fields.go` around lines 26 - 30, The migration
adds a plain-text site field cfApiToken (core.TextField with Name "cfApiToken")
which stores a long-lived Cloudflare API token unencrypted; remove this field
from the site schema and instead persist the token in a server-side secret store
or an encrypted-at-rest secrets table with strict access controls, or store only
a non-sensitive site identifier and fetch the token at runtime from a secure
service; update the migration that creates the core.TextField entry for
"cfApiToken" to instead create or reference the secure secret mechanism and add
notes in the code where the token is retrieved so callers use the secret store
API rather than reading a site field.
Comment on lines
+110
to
+112
| if (cf_token) { | ||
| data.cfApiToken = cf_token | ||
| } |
There was a problem hiding this comment.
Clearing API token from the form does not persist.
cfApiToken is only included when truthy, so an intentionally cleared token never gets written and the previous secret remains.
Proposed fix
- if (cf_token) {
- data.cfApiToken = cf_token
- }
+ // Always persist explicit token edits, including clear-to-empty.
+ data.cfApiToken = cf_token📝 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.
Suggested change
| if (cf_token) { | |
| data.cfApiToken = cf_token | |
| } | |
| // Always persist explicit token edits, including clear-to-empty. | |
| data.cfApiToken = cf_token |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/dashboard/sites/`+page.svelte around lines 110 - 112, The form
only assigns data.cfApiToken when cf_token is truthy, so clearing the token
(empty string or falsy value) doesn't persist and the old secret remains; remove
the truthy guard and always set data.cfApiToken = cf_token (or explicitly set to
''/null when cf_token is empty) wherever the current block assigns cfApiToken so
an intentionally cleared token is written back.
Amerlander
commented
Feb 27, 2026
| import { Globe, Loader, ChevronDown, SquarePen, Trash2, EllipsisVertical, ArrowLeftRight, Download, CirclePlus } from 'lucide-svelte' | ||
| import { Globe, Loader, ChevronDown, SquarePen, Trash2, EllipsisVertical, ArrowLeftRight, Download, CirclePlus, Settings } from 'lucide-svelte' | ||
| import { useSidebar } from '$lib/components/ui/sidebar' | ||
| const sidebar = useSidebar() |
Contributor
Author
There was a problem hiding this comment.
I had to add this to make the dashboard load at all, its not actually part of the new functionality of this PR
Amerlander
commented
Feb 27, 2026
|
|
||
| const site_group_id = $derived(page.url.searchParams.get('group')) | ||
| import { toast } from 'svelte-sonner' | ||
| const site_group_id = $derived(page.url.searchParams.get('group') ?? '') |
Contributor
Author
There was a problem hiding this comment.
Same for this change
- Wrangler version (npm handles version) and add command timeout - Do not return raw deploy command output to clients - Do not expose cfApiToken in the shared client Site schema
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/routes/dashboard/sites/+page.svelte (1)
111-113:⚠️ Potential issue | 🟠 MajorClearing
cfApiTokenstill cannot be persisted.The truthy guard prevents writing an intentional empty token, so previous secret values remain.
🔧 Proposed fix
- if (cf_token) { - data.cfApiToken = cf_token - } + // Persist explicit edits, including clear-to-empty. + data.cfApiToken = cf_token🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/dashboard/sites/`+page.svelte around lines 111 - 113, The current truthy guard around cf_token prevents persisting an intentional empty/cleared token so old secrets remain; update the assignment logic around cf_token so data.cfApiToken is set whenever cf_token is present in the request (i.e., distinguish undefined from empty string) — remove the truthy-only check and instead check for cf_token !== undefined (or explicitly handle empty string) and assign to data.cfApiToken accordingly (referencing cf_token and data.cfApiToken in the +page.svelte handler).internal/export.go (1)
91-92:⚠️ Potential issue | 🔴 CriticalCentralize strict
hostvalidation before any filesystem/prefix usage.
hostis still used unsafely in ZIP/deploy-status flows, and deploy validation remains partial. This can permit unintended path resolution.🔒 Proposed hardening
+validateHost := func(host string) bool { + return host != "" && + !filepath.IsAbs(host) && + !strings.Contains(host, "..") && + !strings.ContainsAny(host, `/\`) +} ... - buf, err := buildSiteZip(pb, site.GetString("host")) + host := site.GetString("host") + if !validateHost(host) { + return e.BadRequestError("invalid host name", nil) + } + buf, err := buildSiteZip(pb, host) ... - host := site.GetString("host") - siteDir := filepath.Join(e.App.DataDir(), "storage", "sites", host) + host := site.GetString("host") + if !validateHost(host) { + return e.BadRequestError("invalid host name", nil) + } + siteDir := filepath.Join(e.App.DataDir(), "storage", "sites", host) ... - if strings.Contains(host, "/") || strings.Contains(host, "\\") || host == ".." { + if !validateHost(host) { return e.BadRequestError("invalid host name", nil) }Also applies to: 116-118, 329-332
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/export.go` around lines 91 - 92, Centralize strict validation and sanitization of the host value before any filesystem or prefix usage: validate the value returned by site.GetString("host") once at the entry point and reject or normalize hosts containing path-separators, dot-segments, nulls, absolute paths, or any characters that could cause path traversal; then pass the validated/sanitized host into buildSiteZip and all deploy-status/ZIP flows instead of calling site.GetString("host") inline. Update the code paths that currently call buildSiteZip(pb, site.GetString("host")) and any deploy-status logic that uses the raw host (the ZIP/deploy-status flows mentioned in the review) to use the single validated variable and return an error early for invalid hosts. Ensure unit tests cover rejected/normalized host values and that logs include the rejection reason.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/export.go`:
- Around line 244-267: The os.Stat call for siteDir can return non-ENOENT errors
leaving infoDir nil and causing a panic when accessing infoDir.ModTime(); update
the handler around os.Stat(siteDir) to explicitly handle three cases: (1) if
os.IsNotExist(err) return the "exists: false" response as before, (2) if err !=
nil (non-ENOENT) return an error/500 JSON response or log and treat as "exists:
false"—but do not continue using infoDir, and (3) on success set infoDir and
continue; also ensure before using infoDir.ModTime() (and when setting
lastGenerated fallback) that infoDir is non-nil and lastGenerated is properly
set after filepath.Walk by referencing siteDir, lastGenerated, infoDir, and
filepath.Walk.
In `@src/lib/components/Modals/Deploy/CFDeploy.svelte`:
- Around line 168-171: The anchor element rendering the external deployment link
in CFDeploy.svelte (the <a href={deployUrl} target="_blank" class="deploy-link">
element that wraps {deployUrl} and <Icon ... />) is missing rel="noopener
noreferrer"; update that anchor to include rel="noopener noreferrer" to prevent
tabnabbing/vulnerability while preserving target="_blank" and the existing
classes and Icon component.
---
Duplicate comments:
In `@internal/export.go`:
- Around line 91-92: Centralize strict validation and sanitization of the host
value before any filesystem or prefix usage: validate the value returned by
site.GetString("host") once at the entry point and reject or normalize hosts
containing path-separators, dot-segments, nulls, absolute paths, or any
characters that could cause path traversal; then pass the validated/sanitized
host into buildSiteZip and all deploy-status/ZIP flows instead of calling
site.GetString("host") inline. Update the code paths that currently call
buildSiteZip(pb, site.GetString("host")) and any deploy-status logic that uses
the raw host (the ZIP/deploy-status flows mentioned in the review) to use the
single validated variable and return an error early for invalid hosts. Ensure
unit tests cover rejected/normalized host values and that logs include the
rejection reason.
In `@src/routes/dashboard/sites/`+page.svelte:
- Around line 111-113: The current truthy guard around cf_token prevents
persisting an intentional empty/cleared token so old secrets remain; update the
assignment logic around cf_token so data.cfApiToken is set whenever cf_token is
present in the request (i.e., distinguish undefined from empty string) — remove
the truthy-only check and instead check for cf_token !== undefined (or
explicitly handle empty string) and assign to data.cfApiToken accordingly
(referencing cf_token and data.cfApiToken in the +page.svelte handler).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
devenv.nixinternal/export.gosrc/lib/builder/views/editor/Toolbar.sveltesrc/lib/common/models/Site.tssrc/lib/components/Modals/Deploy/CFDeploy.sveltesrc/routes/dashboard/sites/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/common/models/Site.ts
Contributor
Author
|
Implemented CodeRabbit suggestions:
Added Branching for Cloudflare Deployment: 
Added Outdated Check: No Preview: A warning is shown with a "Generate Preview" button. Deployment is disabled until preview is generated. Old Preview: A warning shows the age of the preview. Buttons for "Generate Fresh Preview" and "Deploy Anyway" are provided. Generating: A loading spinner is shown while the preview is being generated. Initial: Once a fresh preview is available, the standard deployment configuration (branch selection) is shown. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/builder/views/editor/Toolbar.svelte`:
- Around line 52-57: deploy_in_progress is never toggled so the Deploy button's
loading state won't show; in handle_deploy() set deploy_in_progress = true at
the start of the async operation (and cf_deploying = true if still needed),
await the deploy call, then set deploy_in_progress = false and cf_deploying =
false in a finally block (also set deploy_in_progress = false on early return);
update any error/return paths to ensure both flags are cleared so the button's
loading state reflects the real deploy lifecycle (refer to deploy_in_progress,
cf_deploying, and handle_deploy).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/builder/views/editor/Toolbar.sveltesrc/lib/components/Modals/Deploy/CFDeploy.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/components/Modals/Deploy/CFDeploy.svelte
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/components/Modals/Deploy/CFDeploy.svelte`:
- Around line 8-9: Normalize and validate the branch value before any URL
construction or deploy request: trim leading/trailing whitespace from the branch
state and if the result is empty fallback to 'main' (apply this normalization
wherever the branch is read—e.g., where the branch variable is used to build
preview URLs, request payloads, or displayed). Update usages in CFDeploy.svelte
that reference branch (including the initial state/default, the URL construction
logic, any fetch/deploy payload creation, and UI rendering sections) to use the
normalizedBranch (or call a small normalizeBranch helper) so you never send or
render empty/whitespace-only branch names.
- Around line 42-45: The catch block for the preview-status check erroneously
resets the deployment flow by setting stage = 'INITIAL'; instead, do not revert
to INITIAL on network/auth/API errors—leave the current stage unchanged or set a
distinct error state (e.g., stage = 'PREVIEW_STATUS_ERROR' or
'STATUS_CHECK_ERROR') and keep the console.error(err) so the UI can surface the
failure and prevent progressing to deploy-ready; update the catch in
CFDeploy.svelte where the preview status is checked (the block that currently
logs 'Status check error:' and assigns stage = 'INITIAL') to use a dedicated
error state or no-op the stage change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/export.gosrc/lib/builder/views/editor/Toolbar.sveltesrc/lib/components/Modals/Deploy/CFDeploy.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/export.go
Comment on lines
+8
to
+9
| let branch = $state('main') | ||
| let stage = $state('CHECKING') // CHECKING, INITIAL, DEPLOYING, DEPLOYED, ERROR, NO_PREVIEW, OLD_PREVIEW, GENERATING |
There was a problem hiding this comment.
Normalize and validate branch before URL/render/deploy usage.
branch is used raw in URL construction and request payload. Leading/trailing spaces (or empty values) produce invalid targets and avoidable deploy failures.
Suggested fix
let branch = $state('main')
+ const normalized_branch = $derived(branch.trim())
@@
const targetUrl = $derived.by(() => {
if (!site.cfProjectName) return ''
- if (branch === 'main') return `https://${site.cfProjectName}.pages.dev`
- return `https://${branch}.${site.cfProjectName}.pages.dev`
+ if (!normalized_branch) return ''
+ if (normalized_branch === 'main') return `https://${site.cfProjectName}.pages.dev`
+ return `https://${normalized_branch}.${site.cfProjectName}.pages.dev`
})
@@
async function handle_deploy() {
+ if (!normalized_branch) {
+ error = 'Branch is required'
+ stage = 'ERROR'
+ return
+ }
stage = 'DEPLOYING'
error = null
@@
- body: JSON.stringify({ branch })
+ body: JSON.stringify({ branch: normalized_branch })
})Also applies to: 17-19, 61-72, 148-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/Modals/Deploy/CFDeploy.svelte` around lines 8 - 9,
Normalize and validate the branch value before any URL construction or deploy
request: trim leading/trailing whitespace from the branch state and if the
result is empty fallback to 'main' (apply this normalization wherever the branch
is read—e.g., where the branch variable is used to build preview URLs, request
payloads, or displayed). Update usages in CFDeploy.svelte that reference branch
(including the initial state/default, the URL construction logic, any
fetch/deploy payload creation, and UI rendering sections) to use the
normalizedBranch (or call a small normalizeBranch helper) so you never send or
render empty/whitespace-only branch names.
Comment on lines
+42
to
+45
| } catch (err) { | ||
| console.error('Status check error:', err) | ||
| stage = 'INITIAL' // Fallback to initial if check fails | ||
| } |
There was a problem hiding this comment.
Do not fall back to INITIAL when preview-status check fails.
On Lines 42-45, network/auth/API errors are treated as deploy-ready state. That bypasses the preview precondition and can send users down an invalid deploy path.
Suggested fix
} catch (err) {
console.error('Status check error:', err)
- stage = 'INITIAL' // Fallback to initial if check fails
+ error = 'Unable to verify preview status. Please retry.'
+ stage = 'ERROR'
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/Modals/Deploy/CFDeploy.svelte` around lines 42 - 45, The
catch block for the preview-status check erroneously resets the deployment flow
by setting stage = 'INITIAL'; instead, do not revert to INITIAL on
network/auth/API errors—leave the current stage unchanged or set a distinct
error state (e.g., stage = 'PREVIEW_STATUS_ERROR' or 'STATUS_CHECK_ERROR') and
keep the console.error(err) so the UI can surface the failure and prevent
progressing to deploy-ready; update the catch in CFDeploy.svelte where the
preview status is checked (the block that currently logs 'Status check error:'
and assigns stage = 'INITIAL') to use a dedicated error state or no-op the stage
change.
|
any update on this? |
Contributor
Author
|
I am not a maintainer, so I don't know if that is even considered for merging or if these features go against the planned development. |
Collaborator
|
Thanks for this @Amerlander I'm close to done with the CLI, which will enable building+deploying the site to any host from the terminal. I think that's a better approach compared to adding provider-specific UI/functionality to the codebase. Will leave this for now though into I get y'all's thoughts on the CLI |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I added functionality to the download button and added option to configure cloudflare pages for deployment
When Cloudflare Settings are added to a page the regular deployment can work as a preview. Another button will appear for deployment to cloudflare.
Site first has to be generated; the Cloudflare deployment only takes the pregenerated files that are already present from preview generation.
Page Header without Cloudflare configured:
And with Cloudflare:
Dashboard Context menu without Cloudflare configured:
And with cloudflare Configured:
Cloudflare Configuration:
Deployment toast:
New fields in sites collection:
Usercases:
Summary by CodeRabbit