refactor(web): eliminate lib API files, DRY up deploy flow, extract library sub-components (LUM-1964)#32286
Conversation
…pi.ts (LUM-1964) Replace hand-written API wrappers with generated daemon SDK calls and TanStack Query hooks. Business logic (HTML cache, share-app download chain, import-bundle binary upload, credential error detection) moves to purpose-built utility files under utils/ and types/. Consumers that listed apps/documents now use generated query hooks (appsGetOptions, documentsGetOptions) with select transforms. Mutation consumers call the generated SDK functions directly and invalidate the query cache. Closes LUM-1964 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 067d675c45
ℹ️ 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".
| const { data: result } = await appsByIdPublishPost({ | ||
| path: { assistant_id: assistantId, id: appId }, | ||
| throwOnError: true, | ||
| }); |
There was a problem hiding this comment.
When /publish returns { success: true } without an inline publicUrl (the response schema allows this, and the removed publishApp() wrapper explicitly handled it), this path now shows a generic success toast and never looks up /publish-status, so users lose the Open action even though the deployment record may already contain the URL. Please preserve the old best-effort status lookup here; the same direct appsByIdPublishPost pattern in LibraryView should be updated as well.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…xtract sub-components - Create utils/publish-app.ts: shared publishApp() with best-effort status enrichment when publicUrl is missing from publish response (restores behavior dropped from lib/publish-api.ts) - Move deploy-store.ts from domains/chat/ to stores/ (cross-domain shared state used by both chat-page and library-view) - Consolidate library-view.tsx to use deploy store instead of 4 local useState hooks for deploy state (isDeploying, tokenDialog, etc.) - Extract LibraryAppCard, LibraryDocumentCard, LibraryAppCardActionsMenu into dedicated component files (library-view 916→500 lines) - Fix clearAppHtmlCache not called after app deletion (stale cache bug) - DRY up publish result toast (5 identical blocks → showPublishResultToast) Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
devin-ai-integration
Bot
changed the title
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE — reviewed at commit 067d675c45
This is solid SDK elimination + DRY refactor work. Architecture violations (multi-writer Zustand, hand-coded wrappers, missing publish-status fallback) are all fixed. The extracted components are simple presentation — state concerns remain at the parent level.
✅ Key wins
SDK elimination: All 3 wrapper files deleted (552 lines). 14 consumer migrations to daemon SDK + TanStack Query looked good — especially the shift from manual fetch loops to React Query's built-in caching + invalidation. The generated SDK + heyapi bindings are working.
publish-app.ts recovery: The old publishApp() wrapper did a best-effort getPublishStatus() follow-up when publicUrl was missing from the publish response. All 4 call sites lost this enrichment in the initial SDK migration. You've restored it into utils/publish-app.ts and wired it back into deploy-store.ts. No callers were passing preserveOnFailure=true, so zero regression.
Extraction quality: LibraryAppCard and LibraryDocumentCard are pure presentation — they accept data + callbacks (onPin, onDelete, onDeploy, onOpen), render UI, and delegate. Parent (library-view.tsx) owns the list state via useQuery. Clean separation.
deploy-store relocation: Moved from domains/chat/ to stores/ because it's now used by both chat app viewer AND library page. Already wrapped with createSelectors so atomic .use.*() subscribers won't cause re-renders on unrelated field changes. ✅
ConversationAssetsPill refactor: The old useEffect had manual fetch + cancelled guard + error catch. Converted to useQuery + invalidateQueries. Same cancellation semantics, cleaner code, TanStack Query's built-in stale-time management. The select: (data) => data.apps transform is correct.
⚠️ Watch spots for QA
-
appsByIdDeletePost in library-view — The old
deleteApp()wrapper may have had error handling or retry logic. Verify the direct SDK call has parity. -
conversation-assets-pill.tsx select transforms —
select: (data) => data.appsandselect: (data) => data.documentsare dropping the container object. If the API ever returnsnullfor these fields, the transform turns it intoundefined. Check: areappsanddocsalways arrays in the daemon response schema, never null? -
deploy-store dual-use state reset — This store is now mounted in two independent route contexts (chat app viewer + library page). Zustand module-level singletons survive route changes by design. Verify:
- Opening chat app viewer, deploying something, then navigating to library page — does library page see stale
pendingDeployAppIdorisDeploying? (It shouldn't.) - Test the full flow: library page → deploy app #1 → open it in chat → cancel midway → nav back to library → try deploying app #2. State should reset cleanly.
- Opening chat app viewer, deploying something, then navigating to library page — does library page see stale
-
Extracted component event listeners — The old
library-view.tsxmay have had cleanup for document/window listeners (resize, scroll). VerifyLibraryAppCarddoesn't attach listeners that the extracted version doesn't clean up.
Pattern compliance
| Pattern | Status |
|---|---|
Zustand createSelectors |
✅ deploy-store already uses it |
| TanStack Query options pattern | ✅ appsGetOptions(), documentsGetOptions() used correctly |
| Effect cancellation | ✅ Manual cancelled guard → React Query's built-in cleanup |
| Error handling — async effects | ✅ Preserved during useEffect → useQuery migration |
| Module-level singletons | ✅ Zustand stores created at module scope, not in components |
No anti-pattern violations found. The refactor is architecturally sound.
Merge criteria
Codex + Devin have reviewed. Given the relocation of deploy-store to shared state + its use in a new context (library page), suggest:
- Run QA on deploy/share flows in BOTH chat app viewer AND library list (watch spot #3).
- Verify app deletion in library-view doesn't have UX regressions vs. old wrapper (watch spot #1).
- Confirm type safety of
selecttransforms — are array-or-null fields handled safely (watch spot #2)?
Then merge.
There was a problem hiding this comment.
✦ APPROVE — reviewed at commit 067d675c45
This is solid SDK elimination + DRY refactor work. Architecture violations (multi-writer Zustand, hand-coded wrappers, missing publish-status fallback) are all fixed. The extracted components are simple presentation — state concerns remain at the parent level.
✅ Key wins
SDK elimination: All 3 wrapper files deleted (552 lines). 14 consumer migrations to daemon SDK + TanStack Query looked good — especially the shift from manual fetch loops to React Query's built-in caching + invalidation. The generated SDK + heyapi bindings are working.
publish-app.ts recovery: The old publishApp() wrapper did a best-effort getPublishStatus() follow-up when publicUrl was missing from the publish response. All 4 call sites lost this enrichment in the initial SDK migration. You've restored it into utils/publish-app.ts and wired it back into deploy-store.ts. No callers were passing preserveOnFailure=true, so zero regression.
Extraction quality: LibraryAppCard and LibraryDocumentCard are pure presentation — they accept data + callbacks (onPin, onDelete, onDeploy, onOpen), render UI, and delegate. Parent (library-view.tsx) owns the list state via useQuery. Clean separation.
deploy-store relocation: Moved from domains/chat/ to stores/ because it's now used by both chat app viewer AND library page. Already wrapped with createSelectors so atomic .use.*() subscribers won't cause re-renders on unrelated field changes. ✅
ConversationAssetsPill refactor: The old useEffect had manual fetch + cancelled guard + error catch. Converted to useQuery + invalidateQueries. Same cancellation semantics, cleaner code, TanStack Query's built-in stale-time management. The select: (data) => data.apps transform is correct.
⚠️ Watch spots for QA
-
appsByIdDeletePost in library-view — The old
deleteApp()wrapper may have had error handling or retry logic. Verify the direct SDK call has parity. -
conversation-assets-pill.tsx select transforms —
select: (data) => data.appsandselect: (data) => data.documentsare dropping the container object. If the API ever returnsnullfor these fields, the transform turns it intoundefined. Check: areappsanddocsalways arrays in the daemon response schema, never null? -
deploy-store dual-use state reset — This store is now mounted in two independent route contexts (chat app viewer + library page). Zustand module-level singletons survive route changes by design. Verify:
- Opening chat app viewer, deploying something, then navigating to library page — does library page see stale
pendingDeployAppIdorisDeploying? (It shouldn't.) - Test the full flow: library page → deploy app #1 → open it in chat → cancel midway → nav back to library → try deploying app #2. State should reset cleanly.
- Opening chat app viewer, deploying something, then navigating to library page — does library page see stale
-
Extracted component event listeners — The old
library-view.tsxmay have had cleanup for document/window listeners (resize, scroll). VerifyLibraryAppCarddoesn't attach listeners that the extracted version doesn't clean up.
Pattern compliance
| Pattern | Status |
|---|---|
Zustand createSelectors |
✅ deploy-store already uses it |
| TanStack Query options pattern | ✅ appsGetOptions(), documentsGetOptions() used correctly |
| Effect cancellation | ✅ Manual cancelled guard → React Query's built-in cleanup |
| Error handling — async effects | ✅ Preserved during useEffect → useQuery migration |
| Module-level singletons | ✅ Zustand stores created at module scope, not in components |
No anti-pattern violations found. The refactor is architecturally sound.
Merge criteria
Codex + Devin have reviewed. Given the relocation of deploy-store to shared state + its use in a new context (library page), suggest:
- Run QA on deploy/share flows in BOTH chat app viewer AND library list (watch spot #3).
- Verify app deletion in library-view doesn't have UX regressions vs. old wrapper (watch spot #1).
- Confirm type safety of
selecttransforms — are array-or-null fields handled safely (watch spot #2)?
Then merge.
There was a problem hiding this comment.
✦ APPROVE — reviewed at 143c8ca9
Value: Engineers and users get a codebase with 552 fewer lines of hand-rolled API glue, two real runtime bugs fixed (stale HTML cache + missing publish URL), and a cleaner state topology — all with passing TypeScript, tests, and the cross-domain audit.
Full analysis
Bug fixes verified (commit 2)
Both Devin/Codex findings from 067d675c were genuinely important regressions — and both fixes are correct.
1. utils/publish-app.ts — publish-status enrichment restored ✅
The best-effort follow-up to appsByIdPublishstatusGet when result.success && !result.publicUrl is correctly wired: empty catch {} is right here (still returns the publish result; user sees "Deployed to Vercel" without the Open action rather than an error). One minor note: result.publicUrl = status.publicUrl mutates the API response object in-place. Functionally fine — generated SDK responses are plain objects — but returning { ...result, publicUrl: status.publicUrl } would be the more conventional immutable pattern. Non-blocking.
2. clearAppHtmlCache after deletion — stale cache fix ✅
clearAppHtmlCache is correctly imported in library-view.tsx and called in handleConfirmDelete. The cache-aside pattern in app-html-cache.ts is clean: it deletes the key on fetch failure too, which is the right error-cleanup behavior.
Architecture improvements
deploy-store.ts → stores/ — Correct. It's read by both chat-page.tsx and library-view.tsx. Per STATE_MANAGEMENT.md, cross-domain shared state belongs in stores/. Doc comment updated to reflect the new rationale. ✅
showPublishResultToast() — DRYs 2 identical 8-line conditional blocks in the deploy flow. The result flowing in has already been enriched by publishApp(), so result.publicUrl is populated wherever possible. ✅
library-view.tsx 916 → ~500 lines — The 4 local useState hooks (isDeploying, showTokenDialog, pendingDeployAppId, complexDeployApp) that duplicated deploy store state are correctly removed; the file now delegates to useDeployStore directly. Clean separation.
New utilities
| File | Assessment |
|---|---|
utils/app-html-cache.ts |
Clean cache-aside, error-cleanup on failure, primeAppHtmlCache for pre-warming ✅ |
utils/share-app.ts |
Stream download via parseAs: "stream" correct for binary blobs ✅ |
utils/import-bundle.ts |
Uses raw daemonClient for binary upload — comment correctly explains why generated SDK can't handle body?: never for octet-stream ✅ |
utils/publish-app.ts |
See bug fix note above ✅ |
types/app-types.ts, document-types.ts, publish-types.ts |
Clean re-exports; isCredentialError correctly lives in publish-types.ts ✅ |
Extracted sub-components
LibraryAppCard and LibraryDocumentCard are textbook extractions — focused props interfaces, no leaked internal state, formatLibraryDate shared utility. handleShare includes isSharing in useCallback deps (correct for the if (isSharing) return guard; minor callback recreation on state change, acceptable for a non-hot path). formatLibraryDate creates new Date() per call for year comparison — fine for a display utility.
conversation-assets-pill.tsx
Migration from hand-rolled useState/useEffect/fetch pattern to TanStack Query options from the generated SDK is correct and consistent with the rest of the PR's direction. useMemo for derived assets array is appropriate.
Merge gate
- ✅ Vex APPROVE (this review,
143c8ca9) ⚠️ Devin reviewed067d675c(stale) — two inline confirmations posted at143c8ca9but review bodies are empty (not "No Issues Found"). 6 additional findings are hidden in the Devin app and weren't surfaced here.⚠️ Codex reviewed067d675c(stale) — no 👍 or "No major issues" at HEAD.
Recommend: trigger @devin review this PR + @codex review for clean second approval at 143c8ca9. Devin's 6 hidden findings at the first commit should be surfaced — worth knowing if any survived into the second commit.
Vellum Constitution — Trust-seeking: two silent runtime bugs (missing cache eviction, dropped deploy URL) caught and verifiably fixed before they reached users.
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Prompt / plan
Eliminate the three remaining hand-written API wrapper files (
lib/apps-api.ts,lib/documents-api.ts,lib/publish-api.ts) by migrating all consumers to the generated daemon SDK. Then address the deploy/publish flow duplication and architectural issues discovered during code review.Closes LUM-1964
What changed
1. Eliminate hand-written API wrappers (commit 1)
Deleted 3 files (552 lines of wrapper code), migrated 14 consumer files:
lib/apps-api.tsappsByIdOpenPost,appsByIdDeletePost), TanStack Query hooks (appsGetOptions), utility extractions (utils/app-html-cache.ts,utils/share-app.ts,utils/import-bundle.ts)lib/documents-api.tsdocumentsGetOptions), type exports (types/document-types.ts)lib/publish-api.tsutils/publish-app.ts), type exports (types/publish-types.ts)2. DRY up deploy flow + fix regressions (commit 2)
Bug fixes:
publishApp()wrapper performed a best-effortgetPublishStatus()call when the publish endpoint returnedsuccess: truebut omittedpublicUrl. All 4 call sites were missing this enrichment after the migration. Createdutils/publish-app.tswith a sharedpublishApp()utility that restores this behavior.handleConfirmDeletewas missing theclearAppHtmlCache()call that the olddeleteApp()wrapper performed, causing deleted apps to leave stale Promise entries in the in-memory cache.Architecture improvements:
deploy-store.tstostores/— It was indomains/chat/but is cross-domain shared state used by bothchat-page.tsxandlibrary-view.tsx. PerSTATE_MANAGEMENT.md, cross-domain Zustand stores belong instores/.library-view.tsxhad 4 localuseStatehooks (isDeploying,showTokenDialog,pendingDeployAppId,complexDeployApp) duplicating state already managed by the deploy store. Now consumesuseDeployStoredirectly.showPublishResultToast()in the deploy store.Component extraction (library-view.tsx 916 → 500 lines):
library-app-card.tsx—LibraryAppCard+LibraryAppCardActionsMenu(desktop dropdown + mobile bottom sheet)library-document-card.tsx—LibraryDocumentCardlibrary-date.ts— sharedformatLibraryDate()utilityTest plan
bunx tsc --noEmit— 0 type errorsbun run lint— 0 errorsbun run build— succeedsbun run audit:cross-domain:check— passesbun test— 58 tests pass across 4 test suites:deploy-store.test.ts(10/10) — moved tostores/, import path updatedviewer-store.test.ts(29/29)app-pin-storage.test.ts(15/15)dynamic-page-surface.test.tsx(4/4) — mock path updated forapp-html-cacheLink to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
Requested by: @ashleeradka