Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions hosting/pkg/src/endpoints/appGet.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { OpenAPIRoute, contentJson } from "chanfana";
import { Context } from "hono";
import { z } from "zod";

// Public metadata response - no sensitive data
const AppMetadataSchema = z.object({
slug: z.string(),
title: z.string().optional(),
remixOf: z.string().nullable().optional(),
hasScreenshot: z.boolean(),
hasIcon: z.boolean(),
});

export class AppGet extends OpenAPIRoute {
schema = {
tags: ["Apps"],
summary: "Get app metadata by slug",
request: {
params: z.object({
slug: z.string(),
}),
},
responses: {
"200": {
description: "Returns app metadata",
...contentJson(
z.object({
success: z.boolean(),
app: AppMetadataSchema,
}),
),
},
"404": {
description: "App not found",
...contentJson(
z.object({
error: z.string(),
}),
),
},
},
};

async handle(c: Context<{ Bindings: Env }>) {
const { slug } = c.req.param();
const kv = c.env.KV;

// Fetch app from KV by slug
const appData = await kv.get(slug);

if (!appData) {
return c.json({ error: "App not found" }, 404);
}

const app = JSON.parse(appData);

// Return only public metadata - no code, userId, or chatId
return c.json({
success: true,
app: {
slug: app.slug,
title: app.title || slug,
remixOf: app.remixOf || null,
hasScreenshot: app.hasScreenshot || false,
hasIcon: app.hasIcon || false,
},
Comment on lines +55 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

kv.get(slug) returns a string that is parsed directly with JSON.parse without any validation or defensive handling. If the KV entry is malformed JSON or not in the expected shape, this will throw, returning a 500 instead of a controlled 4xx/5xx response and bypassing your AppMetadataSchema. This is an example of unsafe but type-valid JSON parsing the instructions warn about.

Since this is a public-facing endpoint, consider validating the parsed value against AppMetadataSchema (or a backend version of it) and handling parse/validation errors gracefully so the API contract stays stable and you don’t accidentally expose unexpected fields if the stored schema changes.

Suggestion

You could wrap the JSON parse in a try/catch and validate with Zod before responding, e.g.:

let appJson: unknown;
try {
  appJson = JSON.parse(appData);
} catch (err) {
  console.error("Failed to parse app data from KV", { slug, err });
  return c.json({ error: "App data corrupted" }, 500);
}

const parseResult = AppMetadataSchema.safeParse(appJson);
if (!parseResult.success) {
  console.error("App data failed validation", { slug, issues: parseResult.error.issues });
  return c.json({ error: "App data invalid" }, 500);
}

const app = parseResult.data;

return c.json({
  success: true,
  app: {
    slug: app.slug,
    title: app.title || slug,
    remixOf: app.remixOf ?? null,
    hasScreenshot: app.hasScreenshot ?? false,
    hasIcon: app.hasIcon ?? false,
  },
});

This keeps the public response shape aligned with AppMetadataSchema and prevents runtime crashes due to bad KV contents. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.

});
}
}
3 changes: 3 additions & 0 deletions hosting/pkg/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { fromHono } from "chanfana";
import { AppCreate } from "./endpoints/appCreate.js";
import { AppGet } from "./endpoints/appGet.js";
import {
ClaudeChat,
ImageEdit,
Expand Down Expand Up @@ -86,6 +87,7 @@ openapi.use("/api/*", async (c, next) => {

// Register OpenAPI endpoints
openapi.post("/api/apps", AppCreate);
openapi.get("/api/apps/:slug", AppGet);

// Register OpenAI image endpoints
openapi.post("/api/openai-image/generate", ImageGenerate);
Expand All @@ -107,6 +109,7 @@ export default {

// Test exports - expose internal modules for testing
export { AppCreate } from "./endpoints/appCreate.js";
export { AppGet } from "./endpoints/appGet.js";
// Re-export from hosting-base
export {
// the only import of these is tests, they should take direct from hosting-base, and then remove these lines
Expand Down
2 changes: 1 addition & 1 deletion prompts/pkg/chat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,5 +215,5 @@ export interface ChatInterfaceProps extends ChatState {
export interface SessionSidebarProps {
isVisible: boolean;
onClose: () => void;
sessionId: string;
sessionId?: string;
}
6 changes: 1 addition & 5 deletions vibes.diy/pkg/app/components/BrutalistLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ export default function BrutalistLayout({
return (
<div className="page-grid-background grid-background min-h-screen min-h-[100svh] min-h-[100dvh] w-full relative">
{/* SessionSidebar */}
<SessionSidebar
isVisible={isSidebarVisible}
onClose={closeSidebar}
sessionId=""
/>
<SessionSidebar isVisible={isSidebarVisible} onClose={closeSidebar} />

{/* Hamburger menu button - fixed top left */}
<div className="absolute top-4 left-4">
Expand Down
31 changes: 27 additions & 4 deletions vibes.diy/pkg/app/routes/vibe-viewer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,34 @@ function VibeInstanceViewerContent() {
const fullId = `${titleId}-${installId}`;
const instanceExists = instances.some((inst) => inst._id === fullId);

// Create instance if it doesn't exist (lazy creation for Fresh Data)
// Pass the installId explicitly to ensure correct _id is created
// Create instance if it doesn't exist - fetch real title from hosting API
if (!instanceExists) {
// Let error throw - no catch handler
createInstance("Fresh Data", {}, installId);
const fetchAndCreateInstance = async () => {
try {
// Fetch app metadata from hosting API
const apiBaseUrl =
import.meta.env.VITE_API_BASE_URL ||
"https://vibes-hosting-v2-preview.jchris.workers.dev";
const response = await fetch(`${apiBaseUrl}/api/apps/${titleId}`);
Comment on lines +65 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

The API base URL is hardcoded to a preview workers domain as a fallback. That’s useful for development but risky in production: if VITE_API_BASE_URL is misconfigured or absent, production clients would start talking to the preview service silently. This is an environment/configuration correctness issue rather than a type error.

It’s usually better for production builds to fail fast (or at least log loudly) when critical configuration is missing instead of silently using a preview URL.

Suggestion

Consider moving the default into a clearly dev-only path or asserting that VITE_API_BASE_URL is defined in production, e.g.:

const apiBaseUrl = import.meta.env.VITE_API_BASE_URL;
if (!apiBaseUrl) {
  if (import.meta.env.DEV) {
    console.warn(
      "VITE_API_BASE_URL not set; falling back to preview hosting endpoint for dev only.",
    );
  } else {
    throw new Error("VITE_API_BASE_URL must be configured in production.");
  }
}

const response = await fetch(`${apiBaseUrl ?? "https://vibes-hosting-v2-preview.jchris.workers.dev"}/api/apps/${titleId}`);

This keeps the convenient dev fallback but prevents silent misrouting in production. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.


let title = titleId; // Fallback to slug if fetch fails
if (response.ok) {
const data = (await response.json()) as {
app?: { title?: string };
};
title = data.app?.title || titleId;
}

// Create instance with real title
await createInstance(title, {}, installId);
} catch (error) {
// If fetch fails, use slug as title
console.warn("Failed to fetch app metadata, using slug:", error);
await createInstance(titleId, {}, installId);
Copy link
Contributor

Choose a reason for hiding this comment

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

The fetch URL is constructed using template interpolation with titleId, which ultimately comes from the route. While that's normal, there are two subtle issues here:

  1. Lack of URL encoding: If titleId ever contains characters that are not URL-safe (spaces, ?, #, etc.), the request could break or be misinterpreted by the server.
  2. Missing timeout / abort: fetch is called without any timeout or AbortController, so a slow or hanging hosting API could block instance creation for an unbounded time and degrade UX on this page.

These are not correctness bugs today given typical slug shapes, but they are easy-to-hit edge cases and a performance/UX risk if the hosting API becomes flaky.

Suggestion

To make this more robust:

  • URL-encode titleId when constructing the path segment, e.g. encodeURIComponent(titleId).
  • Optionally, add a basic timeout via AbortController so a stuck API call doesn't block instance creation indefinitely.

For example:

const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), 8000); // 8s timeout

try {
  const response = await fetch(
    `${apiBaseUrl}/api/apps/${encodeURIComponent(titleId)}`,
    { signal: controller.signal },
  );
  // ... existing logic
} finally {
  clearTimeout(timeoutId);
}

Reply with "@CharlieHelps yes please" if you'd like me to add a commit that applies these changes.

}
};

fetchAndCreateInstance();
Copy link
Contributor

Choose a reason for hiding this comment

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

fetchAndCreateInstance is an async function invoked and then intentionally not awaited inside useEffect. That’s fine for fire-and-forget, but the current implementation doesn’t guard against out-of-order updates: if titleId or installId change while a fetch is in flight, a slow response could still call createInstance with a stale slug/title. This is a subtle correctness concern in the presence of navigation or rapid changes.

React’s recommended pattern is to either track an aborted flag inside the effect or use an AbortController so that only the latest effect run is allowed to call createInstance. Without this, you may end up with duplicate or misnamed instances in edge cases.

Suggestion

You can guard against stale async work by capturing the current titleId/installId and cancelling on cleanup, for example:

useEffect(() => {
  if (!titleId || !installId || isCreating) return;

  const fullId = `${titleId}-${installId}`;
  const instanceExists = instances.some((inst) => inst._id === fullId);
  if (instanceExists) return;

  let cancelled = false;

  const fetchAndCreateInstance = async () => {
    try {
      const apiBaseUrl =
        import.meta.env.VITE_API_BASE_URL ||
        "https://vibes-hosting-v2-preview.jchris.workers.dev";
      const response = await fetch(`${apiBaseUrl}/api/apps/${titleId}`);

      let title = titleId;
      if (response.ok) {
        const data = (await response.json()) as { app?: { title?: string } };
        title = data.app?.title || titleId;
      }

      if (!cancelled) {
        await createInstance(title, {}, installId);
      }
    } catch (error) {
      console.warn("Failed to fetch app metadata, using slug:", error);
      if (!cancelled) {
        await createInstance(titleId, {}, installId);
      }
    }
  };

  void fetchAndCreateInstance();

  return () => {
    cancelled = true;
  };
}, [titleId, installId, instances, createInstance, isCreating]);

This keeps behavior deterministic even if dependencies change mid-flight. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CharlieHelps yes please

Copy link
Contributor

@charliecreates charliecreates bot Dec 3, 2025

Choose a reason for hiding this comment

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

Expand this to see my work.
  • Updated the VibeInstanceViewerContent useEffect in vibes.diy/pkg/app/routes/vibe-viewer.tsx to add a cancelled flag, gate createInstance calls behind it, and return a cleanup function while preserving the existing app metadata fetch flow.
  • Verified the root package.json scripts and confirmed pnpm check is the correct monorepo verification command for changes under vibes.diy/pkg/app/routes.
  • Ran pnpm check and confirmed format, build, lint, and Vitest all pass except the existing vibes.diy/tests/app/remix.test.tsx failure (Remix Route > should process vibe slug and navigate with prompt parameter), which is unrelated to the vibe-viewer.tsx changes.
  • Confirmed there are no new TypeScript errors, lint issues, or snapshot changes attributable to the updated useEffect and its void fetchAndCreateInstance(); usage.
  • Reviewed the updated diff for vibes.diy/pkg/app/routes/vibe-viewer.tsx and ensured the cancellation cleanup function only runs when a new instance needs to be created, matching the original control flow.
  • Committed the vibe-viewer async guard as "fix: guard async instance creation in vibe viewer" and pushed it to the jchris/groups-session branch for PR #714.
  • Validated that the new cancelled guard behaves as a non-breaking safety improvement, so it’s appropriate to reply "@CharlieHelps yes please" and treat the review suggestion as addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Implemented the async cancellation guard around instance creation in vibe-viewer and pushed the change.

  • The useEffect that lazily creates instances now:
    • Tracks a cancelled flag.
    • Wraps createInstance calls in if (!cancelled) checks.
    • Returns a cleanup function that sets cancelled = true so any in-flight fetchAndCreateInstance run won’t create an instance after unmount or dependency changes.
    • Uses void fetchAndCreateInstance() to make the fire‑and‑forget intent explicit.

No other behavior in the effect changed: it still fetches app metadata from the hosting API, falls back to the slug when the fetch fails, and only runs when the instance does not already exist.

I left the separate suggestions about tightening the API base URL fallback and adding URL encoding/timeout unchanged for now, since your “yes please” was on the concurrency comment. Happy to wire those up too if you’d like.

Verification
# Monorepo checks (format, build, lint, tests, hosting tests)
$ pnpm check
  • pnpm check runs:
    • prettier --write (root)
    • core-cli tsc (monorepo build)
    • eslint . plus tsc --noEmit in hosting/pkg
    • vitest --run across all configured projects
    • @vibes.diy/hosting-tests test suite
  • Current status:
    • All linting, build, and hosting tests pass.
    • One browser test is failing locally:
      • vibes.diy/tests/app/remix.test.tsx
        • Case: “Remix Route > should process vibe slug and navigate with prompt parameter”
        • Failure: mockOnNavigate is never called.
    • This failure is in the Remix route test and not in the vibe viewer path I touched in this commit; I have not adjusted that test or its implementation in this change.

}
}, [titleId, installId, instances, createInstance, isCreating]);

Expand Down