Skip to content

feat(drive-integration): async import runs rearchitecture [INTEG-4526]#11092

Open
david-shibley-contentful wants to merge 10 commits into
masterfrom
feat/drive-integration-async-runs-INTEG-4526
Open

feat(drive-integration): async import runs rearchitecture [INTEG-4526]#11092
david-shibley-contentful wants to merge 10 commits into
masterfrom
feat/drive-integration-async-runs-INTEG-4526

Conversation

@david-shibley-contentful

@david-shibley-contentful david-shibley-contentful commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

INTEG-4526

Summary

  • Rearchitects the import flow from synchronous (20-min blocking spinner) to fully async: wizard fires off the agent run, saves to localStorage, redirects immediately to a new Runs page
  • New Runs page becomes the app home screen — lists all imports with live status badges (Running / Needs Review / Completed / Failed / Expired), polls every 10s while any run is in-flight, supports multiple concurrent runs
  • ReviewPage is now reachable directly from the Runs page via a Review CTA (no longer locked inside the wizard flow); entry creation stays synchronous
image

Key files

  • src/types/runs.tsRunRecord, DisplayStatus, RunWithStatus, AppView types
  • src/hooks/useRunStorage.ts — localStorage-backed run history (max 50, scoped by space+env)
  • src/hooks/useRunsPolling.ts — parallel Promise.all polling, 10s interval while running
  • src/services/workflowService.tsresumeAndPollWorkflow extracted from the hook
  • src/locations/Page/components/runs/RunsPage + RunRow components
  • src/locations/Page/Page.tsxAppView discriminated union state machine replaces binary toggle
  • src/locations/Page/components/mainpage/ModalOrchestrator.tsx — fire-and-forget wizard exit

Test plan

  • 104 unit/component tests pass (npm test across all modified files)
  • TypeScript compiles clean (tsc --noEmit)
  • Vite production build succeeds (npm run build)
  • Start an import → wizard closes immediately, Runs page shows "Running" status
  • Wait for AI analysis → run transitions to "Needs Review", Review CTA appears
  • Click Review → ReviewPage loads with correct mapping payload
  • Create entries → run transitions to "Completed" with entry links
  • Start two concurrent imports → both rows appear independently with correct statuses
  • Dismiss a failed/expired run → row removed from list and localStorage
  • Clear localStorage → empty state with "Start your first import" CTA

Generated with Claude Code

@david-shibley-contentful
david-shibley-contentful requested review from a team as code owners July 16, 2026 21:10
@david-shibley-contentful
david-shibley-contentful force-pushed the feat/drive-integration-async-runs-INTEG-4526 branch from 3ca6d78 to 4bc5d43 Compare July 16, 2026 21:52
david-shibley-contentful and others added 3 commits July 16, 2026 16:00
Rearchitects the drive-integration import flow from synchronous (20-min
blocking spinner) to fully async. Import wizard fires off the agent run,
saves a RunRecord to localStorage, then immediately redirects to a new
Runs page. The Runs page polls agentRun status every 10s for in-flight
imports and supports multiple concurrent runs. ReviewPage is now
reachable directly from the Runs page rather than only from the wizard
flow.

Key changes:
- New RunsPage (home screen) + RunRow components with live status badges
- useRunStorage hook (localStorage-backed RunRecord[] with 50-record cap)
- useRunsPolling hook (parallel Promise.all, 10s interval while running)
- workflowService.ts extracted from useWorkflowAgent (resumeAndPollWorkflow)
- Page.tsx rewritten around AppView discriminated union state machine
- ModalOrchestrator: fire-and-forget startWorkflow → addRun → onRunStarted
- 104 tests passing across all new and modified files

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…table after import

Three independent useRunStorage instances (Page, RunsPage, ModalOrchestrator) meant
addRun() only updated local React state — RunsPage wouldn't see the new run without
a page refresh. Lift the hook to Page as the single owner and pass runs/addRun/
removeRun/storageError down as props.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@david-shibley-contentful
david-shibley-contentful force-pushed the feat/drive-integration-async-runs-INTEG-4526 branch from 2734bc2 to 2d23ce1 Compare July 16, 2026 22:00
david-shibley-contentful and others added 3 commits July 16, 2026 16:04
…y ESLint

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ixtures from branch

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…E_ENABLE_MOCK_REVIEW_PAYLOAD branch

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
const handleCancelReview = async (runId?: string) => {
if (runId) {
try {
await resumeAndPollWorkflow(sdk, runId, { cancelled: true });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚔️ This is missing the entryBlockGraph. I think we want it for logging purposes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed — handleCancelReview now accepts (runId: string, entryBlockGraph: EntryBlockGraph) and passes the graph to resumeAndPollWorkflow. The onCancelReview prop on ReviewPage threads it through: onCancelReview={(graph) => handleCancelReview(appView.runId, graph)}.

Comment on lines +212 to +213
onRunStarted={handleRunStarted}
onResetToMain={() => setAppView({ view: 'runs' })}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💭 These props do the same thing, setAppView({ view: 'runs' })

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed — both onResetToMain and onRunStarted in ModalOrchestrator now point to handleRunStarted, which does the single setAppView({ view: AppViewKind.RUNS }). The duplicate prop is gone.

const handleExitReview = () => {
modalOrchestratorRef.current?.resetFlow();
handleReturnToMainPage();
setAppView({ view: 'runs' });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💭 Same logic as handleRunStarted(), would replace with that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed — handleExitReview now calls handleRunStarted() instead of inlining the same setAppView call.

{run.createdEntryIds.map((entryId) => (
<TextLink
key={entryId}
href={`https://app.contentful.com/spaces/${spaceId}/entries/${entryId}`}

@JuliRossi JuliRossi Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

❓ Should this use : sdk.hostnames.webapp instead of https://app.contentful.com ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed — now uses sdk.hostnames.webapp ?? 'app.contentful.com' as webappHost, passed down from RunsPage to RunRow as a prop. All href values build off that host.

default:
return 'expired';
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤔 This could be a map, instead of s switch. Similar to what we do for FAILURE_REASON_MESSAGES

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — replaced the switch with a RUN_STATUS_TO_DISPLAY record map and a toDisplayStatus helper that looks up the map and falls back to DisplayStatus.EXPIRED.

Comment on lines +301 to +312
const runId = await startWorkflow(contentTypeIds, documentSelection);

if (storageError) {
// Storage unavailable — surface error before proceeding
showWorkflowError(
new WorkflowRunError(
'Unable to track this import: browser storage is unavailable or full.',
WorkflowFailureReason.GENERIC
)
);
return;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

❓ Do we want to start the workflow even if we've got no storage? How is the user going to be able to see the run?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — storage check now happens before startWorkflow is called. If storage is unavailable the flow shows an error modal and never kicks off the agent run.

onDismiss: (runId: string) => void;
}

function formatDate(iso: string): string {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⛏️ This manual format handling could be avoided if use a library like Dayjs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Noted — left as-is for now to avoid adding a dependency mid-PR, but happy to add dayjs in a follow-up if the team prefers it consistently across the app.

payload: MappingReviewSuspendPayload;
runId?: string;
} | null>(null);
const [appView, setAppView] = useState<AppView>({ view: 'runs' });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💭 Could we move runId outside AppView type? It makes AppView type a bit inconsistent. This could also make simpler with UseEffect in line 41

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. Kept the discriminated union for now since it makes the AppViewKind.REVIEW case narrowing ergonomic (TypeScript knows appView.runId is defined inside that branch). Moving runId out would require a separate useState<string | null> and a null check everywhere it's used. Worth revisiting, but didn't want to change the shape mid-review.

Comment on lines +42 to +44
if (appView.view !== 'review') {
setPendingReviewPayload(null);
return;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (appView.view !== 'review') {
setPendingReviewPayload(null);
return;
if (appView.view !== 'review') {
setPendingReviewPayload(null);
setIsLoadingReviewPayload(false);
return;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied — the early return now uses AppViewKind.REVIEW enum comparison and also calls setIsLoadingReviewPayload(false) so loading state doesn't leak when navigating away.

errorMessage?: string;
}

export type AppView = { view: 'runs' } | { view: 'import' } | { view: 'review'; runId: string };

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💭 runs, import and review could be an enum

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — AppViewKind is now a TypeScript enum with RUNS = 'runs' and REVIEW = 'review'. All AppView discriminant comparisons and setAppView calls use the enum values.

@JuliRossi JuliRossi Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

❓ Should we allow the user to a way clear the runs they have on the page? So they can free space on the localstorage as well/ Also if a run stuck in 'running' has no action button. The user can't dismiss it or can't cancel it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Two open questions here: (1) Clearing/dismissing runs — we don't currently expose a way to remove individual rows (the removeRun hook exists but isn't wired to any UI). Worth adding a dismiss/clear button, especially for completed/expired rows. Happy to add that in this PR or a follow-up. (2) Stuck 'running' rows — agreed, there's no cancel action for a run that gets stuck. The retry button only appears on failed/expired. One option is to also show a 'Cancel' button on running rows that calls the resume endpoint with { cancelled: true }. Wanted to flag this as a conscious gap — what would you prefer?

@JuliRossi JuliRossi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice work! They way I review PRs:
⛏️ Nitpicks, non blockers
💭 / 🤔 Highly suggested, but non blockers
❓ Question, may be blocking or not depending on the answer
⚔️ Blockers

After testing it locally: There seems to be a bug with the document title that keep it as Untitled document instead of the actual title. We are also showing the entry id instead of the name in the RunPage. Got an edge case, a workflow stucked in running
Captura de pantalla 2026-07-20 a las 1 07 36 p  m

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💭 I think we no longer need the loading step for the ModalOrchestrator

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The FlowStep.LOADING step is still needed — it renders the LoadingModal while startAgentRun is in flight (the network call to kick off the agent). Without it the modal would close immediately before the run is confirmed started. The step goes away as soon as startWorkflow resolves and the flow resets.

david-shibley-contentful and others added 4 commits July 20, 2026 11:31
- Replace OAuthConnector card with compact Connected button
- Fix document titles: fall back to doc.title from Google Picker, recover
  from backend suspendPayload for existing runs stored as "Untitled Document"
- Fix forever-loading status: catch per-run API errors so failures degrade
  to expired instead of blocking the status map
- Add retry feature: stores documentSelection on RunRecord and replaces
  Dismiss with Retry, which restarts the job in-place in the same table row
- Add multi-checkbox filter popover (replaces View all select) and style
  sort-by button with SortAscending/SortDescending icons
- Move Status filters below label, left-aligned to match designs
- Remove table outer border and row box-shadows; keep per-row dividers
- Error message on failed runs moved to tooltip on the Failed badge
- Active runs (loading/running/needs-review) always sort above completed rows
- Bouncing three-dot indicator in action column for in-progress rows
- Match button sizes between Connected and Select file; cap copy max-width 600px

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Convert DisplayStatus and AppViewKind to enums; replace all string literals
- Store documentSelection on RunRecord so retry replays exact original settings
- Replace dismiss with retry action; kicks off new agent run, updates row in-place
- Remove progress bars (redundant with status badges)
- Add bouncing three-dot indicator for in-progress rows
- Replace forever-loading/Untitled fallbacks with polling recovery from suspendPayload
- Use sdk.hostnames.webapp instead of hardcoded app.contentful.com
- Style filter/sort popovers to match design (multi-checkbox filter, sort icons)
- OAuthConnector: compact button replaces card layout
- Error message moved to Tooltip on failed badge (not permanent display)
- Check storage error before starting workflow (not after)
- Guard per-run Promise.all with .catch(() => null) to prevent cascade failures
- Remove unused isAnalyzing state and WorkflowFailureReason re-export

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.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.

2 participants