Skip to content

(feat): Deployments#908

Merged
mcandeia merged 12 commits intomainfrom
ft/indexing-tools
Jul 28, 2025
Merged

(feat): Deployments#908
mcandeia merged 12 commits intomainfrom
ft/indexing-tools

Conversation

@mcandeia
Copy link
Copy Markdown
Contributor

@mcandeia mcandeia commented Jul 25, 2025

feat: Add deployment isolation for hosting apps

Summary

Introduces a new deployment-based architecture for hosting apps to enable breaking change checks and safer production rollouts.

Motivation

Currently, hosting apps are deployed directly without the ability to:

  • Test changes in a preview environment before production
  • Compare deployments for breaking change detection
  • Maintain multiple versions of the same app
  • Rollback to previous versions safely

Changes

Database Schema

  • New table: deco_chat_hosting_apps_deployments

    • id (text): Custom deployment identifier (e.g., "v1.0.0", "preview", "production")
    • hosting_app_id (uuid): FK to hosting apps
    • cloudflare_deployment_id (text): Cloudflare worker identifier
    • files (jsonb): Deployment-specific files
  • Updated table: deco_chat_hosting_routes

    • Changed FK from hosting_app_id to deployment_id
    • Routes now point to specific deployments instead of apps

API Changes

  • Modified updateDatabase() to create deployment records
  • Hosting apps now only store basic metadata (slug, workspace)
  • Deployment-specific data (files, Cloudflare IDs) moved to deployments table
  • Updated routing logic to use deployment-based lookups

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Jul 25, 2025

Deploying chat with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0fb2f42
Status: ✅  Deploy successful!
Preview URL: https://e479b015.chat-46r.pages.dev
Branch Preview URL: https://ft-indexing-tools.chat-46r.pages.dev

View logs

@mcandeia mcandeia force-pushed the ft/indexing-tools branch 2 times, most recently from c53b29f to fff5478 Compare July 25, 2025 14:39
@lucis
Copy link
Copy Markdown
Contributor

lucis commented Jul 25, 2025

bugbot run

@lucis
Copy link
Copy Markdown
Contributor

lucis commented Jul 25, 2025

Amazingggggg

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bugbot free trial expires on July 29, 2025
Learn more in the Cursor dashboard.

Comment thread supabase/migrations/20250622172016_deco_chat_hosting_routes.sql
Comment thread packages/sdk/src/mcp/hosting/api.ts Outdated
},
script: (domain: string) => {
if (domain.endsWith(HOSTING_APPS_DOMAIN)) {
if (domain.endsWith(HOSTING_APPS_DOMAIN) && domain.includes(DOUBLE_DASH)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Domain Format Change Breaks Existing Deployments

The Entrypoint.script method now requires domains to contain DOUBLE_DASH (e.g., app--deploymentid.deco.page) in addition to ending with HOSTING_APPS_DOMAIN. This breaks backward compatibility for existing apps deployed without the new deployment ID format, as their domains will now return null from Entrypoint.script, potentially disrupting routing.

Locations (1)

Fix in CursorFix in Web

Comment thread packages/sdk/src/mcp/hosting/api.ts Outdated
@mcandeia mcandeia force-pushed the ft/indexing-tools branch from f35f542 to 5647ac6 Compare July 25, 2025 18:01
@viktormarinho
Copy link
Copy Markdown
Contributor

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jul 25, 2025

Claude finished @viktormarinho's task —— View job


Code Review Complete ✅

I've reviewed the deployment isolation implementation. This is a significant architectural change that introduces deployment-based hosting with several critical issues that need to be addressed.

Todo List:

  • Read CLAUDE.md for repo-specific guidelines (file doesn't exist)
  • Examine database migrations and schema changes
  • Review core API implementation files
  • Analyze existing bugbot findings and check for additional issues
  • Provide comprehensive review feedback

Summary

The PR introduces a deployment-based architecture for hosting apps, which is a valuable feature for preview environments and rollback capabilities. However, there are several critical bugs and backward compatibility issues that need to be resolved before merging.

Critical Issues (Must Fix)

1. Database Migration Order & Type Mismatch

File: supabase/migrations/20250622172016_deco_chat_hosting_routes.sql:9

The migration creates a foreign key to a table that doesn't exist yet:

  • deco_chat_hosting_routes (20250622172016) references deco_chat_hosting_apps_deployments
  • But deco_chat_hosting_apps_deployments is created later (20250724142558)
  • Type mismatch: deployment_id is uuid but references id which is text

Fix: Either reorder migrations or change the deployment_id column type to text.

2. Backward Compatibility Breaking Change

File: packages/sdk/src/mcp/hosting/api.ts:38

script: (domain: string) => {
  if (domain.endsWith(HOSTING_APPS_DOMAIN) && domain.includes(DOUBLE_DASH)) {
    return domain.split(HOSTING_APPS_DOMAIN)[0];
  }
  return null;
},

This breaks existing deployments without the --deploymentid format. The function now requires DOUBLE_DASH to be present, but existing apps deployed with the old format (e.g., myapp.deco.page) will return null.

Fix: Support both old and new formats:

script: (domain: string) => {
  if (domain.endsWith(HOSTING_APPS_DOMAIN)) {
    return domain.split(HOSTING_APPS_DOMAIN)[0];
  }
  return null;
},

3. Database Constraint Mismatch

File: packages/sdk/src/mcp/hosting/api.ts:249

onConflict: "deployment_id,route_pattern,custom_domain"

The upsert operation specifies a conflict resolution that doesn't match the actual database schema. The schema only has a unique constraint on (route_pattern, custom_domain) for custom domains, not including deployment_id.

Fix: Update to match the actual unique constraint or add the missing constraint to the schema.

Additional Issues Found

4. Domain Ownership Check Logic Gap

File: packages/sdk/src/mcp/hosting/custom-domains.ts:19

const targetAddress = Entrypoint.host(scriptSlug);

This doesn't account for deployment IDs when checking domain ownership. If a domain is configured to point to app--preview.deco.page, the ownership check will fail because it's comparing against app.deco.page.

5. Incomplete File Storage Migration

File: packages/sdk/src/mcp/hosting/api.ts:177-178

// TODO (@mcandeia) files should be stored in R2 instead.
// files,

Files are commented out in deployment creation, which means deployments lose their file data. This breaks the ability to reconstruct or compare deployments.

6. Missing Deployment ID Generation Strategy

The PR generates deployment IDs using uid.rnd() but doesn't provide a way to specify custom deployment IDs (like "production", "staging", "v1.0.0") as mentioned in the PR description.

Architecture Concerns

7. Route Deletion Strategy

File: packages/sdk/src/mcp/hosting/api.ts:216-237

The route diffing logic deletes and recreates routes on each deployment. This could cause brief downtime during deployments and doesn't handle the case where multiple deployments share routes.

8. Missing Rollback Implementation

While the PR mentions rollback capabilities as motivation, there's no implementation of rollback functionality in the API.

Recommendations

  1. Fix Critical Issues First: Address the database migration, backward compatibility, and constraint issues before merging.

  2. Add Migration Strategy: Provide a plan for migrating existing apps to the new deployment-based architecture.

  3. Implement Custom Deployment IDs: Allow users to specify meaningful deployment identifiers.

  4. Complete File Storage: Either implement R2 storage or temporarily store files in the deployment record.

  5. Add Rollback API: Implement the rollback functionality mentioned in the PR motivation.

  6. Add Integration Tests: The deployment system needs comprehensive tests to ensure domain routing, file storage, and rollback work correctly.

Minor Issues

  • File: packages/sdk/src/mcp/hosting/api.ts:672: Inconsistent deployment ID vs deploymentId variable naming
  • Missing error handling for deployment creation failures
  • No cleanup logic for orphaned Cloudflare workers when deployment records fail to create

The concept is solid and addresses real needs, but the implementation has critical bugs that must be resolved before merging to prevent breaking existing production deployments.


mcandeia added 8 commits July 28, 2025 12:56
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
@mcandeia mcandeia force-pushed the ft/indexing-tools branch from 5647ac6 to 80fba56 Compare July 28, 2025 15:56
mcandeia added 4 commits July 28, 2025 13:33
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
@mcandeia
Copy link
Copy Markdown
Contributor Author

approved async w/ @tlgimenes

@mcandeia mcandeia merged commit 556fdbc into main Jul 28, 2025
2 checks passed
@mcandeia mcandeia deleted the ft/indexing-tools branch July 28, 2025 16:49
mcandeia added a commit that referenced this pull request Jul 28, 2025
mcandeia added a commit that referenced this pull request Jul 28, 2025
mcandeia added a commit that referenced this pull request Jul 28, 2025
* Revert "(feat): Deployments (#908)"

This reverts commit 556fdbc.

* Fixes script code

Signed-off-by: Marcos Candeia <marrcooos@gmail.com>

---------

Signed-off-by: Marcos Candeia <marrcooos@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants