Skip to content

feat(web): replace library page stubs with real implementations#31277

Merged
ashleeradka merged 2 commits into
mainfrom
devin/1779286372-lum-1657-port-library-pages
May 20, 2026
Merged

feat(web): replace library page stubs with real implementations#31277
ashleeradka merged 2 commits into
mainfrom
devin/1779286372-lum-1657-port-library-pages

Conversation

@ashleeradka
Copy link
Copy Markdown
Contributor

@ashleeradka ashleeradka commented May 20, 2026

Prompt / plan

Replace the placeholder library pages with real implementations that wire the already-ported LibraryView and AppViewerContainer components (from PR #31271, LUM-1654).

Why needed: The library routes (/assistant/library, /assistant/library/:appId) currently render placeholder stubs with "Placeholder route" text. The actual components they need were ported in LUM-1654 but never wired into the route pages.

What changed:

  • library-page.tsx — Renders LibraryView with assistantId from outlet context, wrapped in the same container styling the platform uses (rounded border, surface overlay background). Provides onNewConversation callback that navigates to the chat route.

  • library-detail-page.tsx — Deep-link page for /assistant/library/:appId. Fetches the app via openApp() on mount, renders AppViewerContainer once loaded, with loading spinner and error states. Close navigates back to the library listing. Uses a request ref to handle concurrent/stale responses (same pattern as use-app-viewer-actions.ts).

Why safe: Additive change replacing inert placeholders. No route changes needed — routes.tsx already had both routes correctly wired. All optional callbacks (onOpenDocument, onEditApp) are omitted for now since they require cross-route state coordination that will come with further migration work. LibraryView handles these gracefully (conditionally hides buttons when callbacks aren't provided).

Alternatives not taken:

  • Rendering LibraryView on the detail page with a pre-opened app: Rejected because the platform's /library/:appId route bypasses the listing entirely and goes straight to the app viewer via the deepLinkAppId flow. A dedicated detail page that loads the app directly is the faithful port of this behavior.
  • Wiring onEditApp (which enters chat editing mode): Requires the chat page's editing state machine and conversation context. This is cross-route coordination that belongs in a future task, not this stub-replacement PR.

Closes LUM-1657

Test plan

  • bunx tsc --noEmit — passes (no type errors)
  • bun run lint — passes
  • bun run build — passes (production bundle succeeds)
  • Routes unchanged in routes.tsx — already correctly wired at /assistant/library and /assistant/library/:appId
  • Runtime testing requires the assistant daemon (not available on CI) — compile-time checks verify all imports resolve, types match, and components compose correctly

Link to Devin session: https://app.devin.ai/sessions/c4696ace57fe43649f2475d7661ac273
Requested by: @ashleeradka


Open in Devin Review

Wire the already-ported LibraryView and AppViewerContainer components
into the library route pages, replacing the placeholder stubs.

- LibraryPage renders LibraryView with assistantId from outlet context,
  matching the platform's rendering wrapper with the same container
  styling (rounded border, surface overlay background).

- LibraryDetailPage is a deep-link page for /assistant/library/:appId
  that fetches the app via openApp() and renders AppViewerContainer
  directly, with loading/error states and back-navigation to the
  library listing.

Routes in routes.tsx were already correctly wired from the initial
scaffolding — no changes needed there.

Closes LUM-1657

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@linear
Copy link
Copy Markdown

linear Bot commented May 20, 2026

LUM-1657

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment on lines +67 to +75
<div className="flex flex-1 flex-col items-center justify-center gap-4">
<p className="text-body-medium-lighter text-[var(--content-tertiary)]">
{error}
</p>
<button
type="button"
onClick={handleClose}
className="text-body-medium-default text-[var(--primary-base)] underline"
>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 Missing catch in handleShare causes unhandled promise rejection and silent failure

The handleShare callback has a try/finally but no catch block. When shareApp throws (e.g. network error, server error), the error propagates as an unhandled promise rejection — the user gets no feedback that the share failed, and an error appears in the console.

Comparison with existing pattern in LibraryView

The analogous handler in library-view.tsx:338-351 correctly catches errors and shows a toast:

catch (err) {
  toast.error("Failed to share app", {
    description: err instanceof Error ? err.message : undefined,
  });
}

It also shows a success toast on the happy path. The LibraryDetailPage version is missing both.

Suggested change
<div className="flex flex-1 flex-col items-center justify-center gap-4">
<p className="text-body-medium-lighter text-[var(--content-tertiary)]">
{error}
</p>
<button
type="button"
onClick={handleClose}
className="text-body-medium-default text-[var(--primary-base)] underline"
>
const handleShare = useCallback(async () => {
if (!assistantId || !app || isSharing) return;
setIsSharing(true);
try {
await shareApp(assistantId, app.appId, app.name);
} catch (err) {
console.error("Failed to share app", err);
} finally {
setIsSharing(false);
}
}, [assistantId, app, isSharing]);
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fixed in 552e005 — added catch (err) with toast.error("Failed to share app", ...) and toast.success("App exported", ...), matching LibraryView's handleShareOpenedApp pattern exactly. Also added unmount cleanup (requestRef.current = null) per the vex-bot note.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cb90b5f73e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +15 to +17
initialMessage
? `${routes.assistant}?message=${encodeURIComponent(initialMessage)}`
: routes.assistant,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Route initial prompt via a consumed launch mechanism

handleNewConversation encodes initialMessage into ?message=..., but the chat route never reads that query parameter, so flows that pass an initial prompt (e.g. the "Let your assistant handle it" deploy confirmation in LibraryView) open chat without the intended seeded message. This makes the new cross-route prompt handoff non-functional; use the existing initial-message launch path (or another consumed mechanism) instead of an unused query key.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch — confirmed the chat route only reads conversationKey and onboarding from search params, never message. Fixed in 552e005 by removing the dead ?message= param. The callback now always navigates to /assistant.

In the platform, onNewConversation calls startNewConversation({ initialMessage }) directly within the same React tree (no navigation). Since the OSS library is a separate route, that function isn't accessible. Left a TODO — the initialMessage seeding requires cross-route state coordination (same category as onEditApp).

Comment on lines +56 to +58
try {
await shareApp(assistantId, app.appId, app.name);
} finally {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Handle share failures to avoid unhandled promise rejections

The share callback awaits shareApp(...) without a catch, so any network/API error rejects out of the click handler. In this case users get no explicit error toast and the app can emit unhandled promise rejection noise in production logs; this is a regression from the existing LibraryView share flow, which catches and reports failures.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fixed in 552e005 — added catch + toast.error/toast.success to match LibraryView's share pattern. The try/finally without catch was a real bug (unhandled rejection + silent failure).

vex-assistant-bot[bot]
vex-assistant-bot Bot previously approved these changes May 20, 2026
Copy link
Copy Markdown
Contributor

@vex-assistant-bot vex-assistant-bot Bot left a comment

Choose a reason for hiding this comment

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

APPROVE

Value: Library users navigating to /assistant/library now see their actual apps instead of "Placeholder route" text — LibraryView and AppViewerContainer are wired with context, loading states, and error handling.

What this does:

  • library-page.tsx — mounts LibraryView with assistantId from outlet context + onNewConversation callback navigating to chat, wrapped in the platform container styling
  • library-detail-page.tsx — loads the app via openApp() on mount, guards stale concurrent navigation via requestRef, shows spinner → AppViewerContainer → error state with back link

Dependency check: #31271 (LUM-1654) merged at 13:21 UTC today — imports resolve correctly ✅

CI: 3/3 ✅ (Lint+Type+Build, Socket PR, Socket Project)

Convention checks: .js extensions ✅ · @/ alias ✅ · kebab-case ✅ · no default exports ✅ · no barrel files ✅ · ApiError thrown correctly in openApp/shareApp ✅ · void navigate() suppresses Promise warning correctly ✅

Capacitor iOS: openApp/shareApp use client.post for JSON responses — no SSE concern. shareApp step 2 uses parseAs: "stream" for a binary bundle download; CapacitorHttp buffers this correctly (not a streaming text/event-stream path). ✅


Non-blocking notes:

  1. useEffect missing unmount cleanuprequestRef.current is never cleared in the effect's cleanup return. If the component unmounts while openApp is in-flight, the ref still matches appId and .then/.catch will call setApp/setError on the dead instance. React 18 makes this a no-op (no warning, no Sentry error), but it diverges from our KB canonical pattern. Suggested minimal fix:

    return () => {
      requestRef.current = null;
    };
  2. handleShare swallows errorstry { await shareApp(...) } finally { setIsSharing(false) } resets the loading state but gives the user no feedback on failure. If shareApp throws, isSharing snaps back to false silently. Acceptable if shareApp handles its own toast internally — worth confirming.

  3. isSharing in handleShare dep array — the guard if (isSharing) return reads isSharing from closure, requiring it in deps, which re-creates handleShare on every state toggle and re-renders AppViewerContainer during the share operation. A useRef guard avoids this churn, but the current approach is functionally correct.

Vellum Constitution — Inviting: replacing a dead-end placeholder with a working library page makes the assistant feel coherent and complete.

…cleanup

- library-detail-page: add catch block + toast for shareApp failures,
  add toast.success on share, add useEffect unmount cleanup for requestRef
- library-page: remove non-functional ?message= query param (chat route
  never reads it); navigate to chat without seeding initial message
  until cross-route state coordination is implemented

Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
@devin-ai-integration
Copy link
Copy Markdown
Contributor

All three non-blocking notes addressed in 552e005:

  1. Unmount cleanup — added return () => { requestRef.current = null; } to the effect.
  2. handleShare error handling — added catch with toast.error + toast.success on success, matching LibraryView's handleShareOpenedApp pattern.
  3. isSharing deps — acknowledged as functionally correct; leaving as-is.

@ashleeradka ashleeradka merged commit 1a8ec9a into main May 20, 2026
3 checks passed
@ashleeradka ashleeradka deleted the devin/1779286372-lum-1657-port-library-pages branch May 20, 2026 14:38
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.

1 participant