fix(web): surface Browse Playbook Exchange in AutoRun empty state#973
Open
chr1syy wants to merge 6 commits intoRunMaestro:rcfrom
Open
fix(web): surface Browse Playbook Exchange in AutoRun empty state#973chr1syy wants to merge 6 commits intoRunMaestro:rcfrom
chr1syy wants to merge 6 commits intoRunMaestro:rcfrom
Conversation
Mobile users can now browse, preview, and import community playbooks from inside the AutoRun setup sheet — closing the AR-PARITY-01 gap between desktop and the mobile interface. - Extracted marketplace cache/fetch/import logic into src/main/services/marketplace-service.ts so the IPC handler and the web-server share one code path (no renderer round-trip). - Added four WebSocket message types: marketplace_get_manifest (with refresh:true), marketplace_get_document, marketplace_get_readme, marketplace_import_playbook. Filename traversal + missing-field validation at the boundary. - Wired callbacks in web-server-factory; the import callback resolves autoRunFolderPath and SSH config from the session itself, so mobile clients can't override them — SSH remote sessions import to the correct host the same way as desktop. - Created MarketplaceSheet (mobile bottom sheet with list → detail → import flow, category chips, search, README/document preview) and added a "Browse Playbook Exchange" entry point at the top of AutoRunSetupSheet. After import, AutoRun docs auto-refresh so freshly imported docs appear in the selector. Tests: new marketplace block in messageHandlers.test.ts covers all four message types + traversal rejection + unconfigured-callback paths. AutoRun App.test mocks the new sheet. All scoped suites pass (marketplace IPC: 45, message handlers: 113, mobile App: 95). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves merge conflict in src/main/ipc/handlers/marketplace.ts by keeping the new thin-wrapper refactor; ports the upstream watcher error-handler addition (Windows UNKNOWN / EPERM / ENOENT classification) into createLocalManifestWatcher in the new src/main/services/marketplace-service.ts. Also addresses PR RunMaestro#947 review feedback from greptile + coderabbit: Security - WS handlers reject `playbookPath` values that point at the local filesystem (absolute or `~`-prefixed). Local paths remain reachable via desktop IPC where the renderer is trusted, but mobile/web clients can no longer coerce the server into reading arbitrary files via the marketplace fetch helpers. - WS `marketplace_import_playbook` rejects `targetFolderName` values containing path separators or traversal tokens. Service layer adds `assertSafeTargetFolderName` as the source-of-truth guard. - IPC `marketplace:importPlaybook` fails loudly when `sshRemoteId` is provided but cannot be resolved, instead of silently downgrading to a local import (matches CLAUDE.md SSH-spawn pattern: never silently downgrade). Correctness - Persisted `newPlaybook.documents` only references docs that actually wrote to disk, so a partial import never produces a playbook pointing at missing files. - Reading the per-session playbooks file no longer silently resets to `[]` on non-ENOENT errors. Corrupt JSON / EACCES now throws rather than overwriting existing user data. - `fetchReadme` re-throws unexpected I/O / network failures (only ENOENT and 404 still map to "no README"), so production faults surface in Sentry. - WS document/readme handlers return a structured failure when the underlying callback resolves to `null`, instead of treating unconfigured wiring as a successful empty response. Code quality - Replaced dynamic `require('os')` in marketplace-service with a static `import os from 'os'` (consistent with the rest of the module). UI polish - AutoRunSetupSheet entry button gets a focus ring on `:focus` so keyboard users still see focus state after `outline: none`. - MarketplaceSheet swaps `||` to `??` for README/document fallback text so empty files render instead of showing the placeholder. - MarketplaceSheet adds a monotonic preview-request id; stale README/document responses are discarded if the user has already picked another playbook or document. Error responses now surface in the import-error banner instead of being swallowed. Tests - Updated marketplace IPC tests for the new SSH fail-loud behavior (replaces "fall back to local fs" cases with "fail loudly"). - Added missing `mockRejectedValueOnce({ code: 'ENOENT' })` for the per-session playbooks file read in 7 tests that previously relied on the bare-catch swallowing JSON.parse errors. - Fixed `os` mock to expose `default` for the new static import. - Asserts that partially-imported playbooks only persist the successful docs. All scoped suites pass (marketplace IPC: 45, message handlers: 113, mobile App: 95). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves 6 conflicts that came in from upstream/rc's PR2 of the CLI surface refactor (list_desktop_sessions / get_session_history) plus the AutoRunSetupSheet playbook-CRUD redesign and FolderPickerSheet: - WebServer.ts, types.ts, CallbackRegistry.ts: combined the marketplace callback registrations with upstream's listDesktopSessions and getSessionHistory callbacks (both sets coexist; ordering preserves marketplace-first since that was already on the branch). - handlers/messageHandlers.ts: combined the four marketplace WS handlers (get_manifest, get_document, get_readme, import_playbook) with the new list_desktop_sessions and get_session_history handlers. - web/mobile/App.tsx: kept both the MarketplaceSheet wiring and the new FolderPickerSheet + handleAutoRunFolderConfirm flow; passed sendRequest, send, currentDocument and onOpenMarketplace through to the redesigned AutoRunSetupSheet. - web/mobile/AutoRunSetupSheet.tsx: kept the upstream playbook-CRUD refactor (sendRequest/send/currentDocument props, playbooks panel, inline name-prompt / delete-confirm modals) and re-anchored the Playbook Exchange entry button above the new Playbooks section so marketplace import remains discoverable. - __tests__/main/web-server/handlers/messageHandlers.test.ts: kept the marketplace mocks/tests alongside upstream's listDesktopSessions / getSessionHistory mocks and tests. Also addresses CI failures and a new coderabbit comment from the last push: CI fix - web-server-factory.test.ts MockWebServer was missing the four setMarketplaceManifestCallback / setMarketplaceDocumentCallback / setMarketplaceReadmeCallback / setImportMarketplacePlaybookCallback stubs, causing 36 test failures with `TypeError: ... is not a function`. Added the stubs (combined with upstream's new setListDesktopSessionsCallback / setGetSessionHistoryCallback). Coderabbit P3 (Major) - Marketplace WS validation branches were emitting generic `type: 'error'` frames rather than the request-scoped `marketplace_*_result` types. Clients waiting on the typed result would miss the failure or hang. Introduced `sendMarketplaceFailure(client, type, error, message, extra?)` helper and routed all validation-failure paths through it (missing fields, untrusted local paths, traversal in filename, separators in targetFolderName, callback unconfigured). Updated the relevant tests (assert typed-result failures and added two new cases for absolute playbookPath rejection and targetFolderName separator rejection). Verification - `npm run lint` clean (TypeScript, all configs). - `npm run lint:eslint` clean. - `prettier --check` clean across all touched files. - Scoped tests pass: marketplace IPC (45), message handlers (123 — 7 new), web-server-factory (95), mobile App (95). 319 total. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The mobile Playbook Exchange prefilled the import folder with a `category/title` slug, which the server's `assertSafeTargetFolderName` guard rejects (no separators allowed). Every user tapping Import on a category-namespaced playbook hit this and had to manually edit the input down to a single segment. Switch the prefill to a title-only slug. Category is already shown via the tile chip and detail-view eyebrow, so re-encoding it into the folder name added no information. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The marketplace sheet's `importError` was overloaded — `handleSelectPlaybook` and `handleSelectDocument` set it on README/document fetch failures, but it rendered in the import footer next to the Import Playbook button. A user who tapped a playbook, saw the preview fail, and read the error in the footer would reasonably believe an import had been attempted and failed. Split into a dedicated `previewError` state that renders inline in the preview area, leaving `importError` exclusively for actual import failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes Gap 3 from the PR RunMaestro#946/RunMaestro#947 mobile/web AutoRun parity follow-up. Desktop has an "Add Docs" affordance that surfaces both Create-doc and Marketplace as co-equal entry points. On mobile/web, the empty state of AutoRunInline only offered "+ Create document" — users with a fresh AutoRun folder had no in-UI path to discover existing playbooks and had to know to open the launch sheet just to find the marketplace entry. - Add onOpenMarketplace?: () => void prop to AutoRunInline. Render a "Browse Playbook Exchange" CTA next to "+ Create document" in the empty state when the prop is provided. Both buttons share the same vertical stack so the discoverability matches desktop's overview. - Plumb the prop through AutoRunPanel (full-screen overlay), AutoRunTabContent / RightDrawer (inline tab), and RightPanel. - Wire handleOpenMarketplaceSheet through from App.tsx for both AutoRunPanel and RightPanel render sites; reuses the same MarketplaceSheet and WS infrastructure introduced by PR RunMaestro#947, so no new IPC/server surface. The CTA is opt-in (prop is optional) so existing callers and tests keep working without change. Existing scoped tests pass; new AutoRunInline.test.tsx covers the empty-state CTA visibility, hidden-when-unset, and onClick behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a complete "Playbook Exchange" (marketplace) feature enabling users to browse, search, and import playbooks on mobile. A new service layer manages manifest caching, document/asset fetching, and playbook imports; IPC/WebSocket handlers delegate to the service; a mobile sheet UI provides discovery and import workflows; and comprehensive tests validate edge cases including SSH remote validation and document filtering. ChangesMarketplace Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR closes Gap 3 from the post-soak parity work: it adds a "Browse Playbook Exchange" CTA to the
Confidence Score: 5/5Safe to merge once #947 lands; the Gap 3 commit is a small, opt-in prop addition on top of well-validated marketplace infrastructure. The net-new commit is minimal — an optional prop threaded through four components with no breaking changes to existing callers or tests. The stacked #947 infrastructure is the larger surface: path-traversal guards appear at both the WS handler layer (isUntrustedLocalPath) and the service layer (assertSafeTargetFolderName / validateSafePath), the import flow is SSH-aware, and the manifest cache uses a conservative 6h TTL. The MarketplaceSheet's monotonic request-ID pattern correctly prevents stale preview responses from clobbering newer selections. No data-loss paths, no missing guards, and the four AutoRunInline tests cover the exact backward-compatibility contract the PR relies on. No files require special attention. The largest new file, MarketplaceSheet.tsx, follows established patterns from the codebase and its async state management is sound. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AutoRunInline
participant App as App.tsx
participant MarketplaceSheet
participant WS as WebSocket Handler
participant MarketplaceService
User->>AutoRunInline: taps "Browse Playbook Exchange"
AutoRunInline->>App: onOpenMarketplace()
App->>App: setShowMarketplaceSheet(true)
App->>MarketplaceSheet: render (sessionId, sendRequest, onImported, onClose)
MarketplaceSheet->>WS: marketplace_get_manifest
WS->>MarketplaceService: getMarketplaceManifest()
MarketplaceService-->>WS: manifest (cached or fetched)
WS-->>MarketplaceSheet: manifest_result
User->>MarketplaceSheet: selects playbook
MarketplaceSheet->>WS: marketplace_get_readme
WS->>MarketplaceService: getMarketplaceReadme(playbookPath)
MarketplaceService-->>WS: readme content
WS-->>MarketplaceSheet: readme_result
User->>MarketplaceSheet: taps "Import Playbook"
MarketplaceSheet->>WS: marketplace_import_playbook (sessionId, playbookId, targetFolderName)
Note over WS: validates sessionId, playbookId,<br/>targetFolderName (no path traversal)
WS->>MarketplaceService: importMarketplacePlaybook()
MarketplaceService-->>WS: importedDocs, importedAssets
WS-->>MarketplaceSheet: import_result (success)
MarketplaceSheet->>App: onImported(folderName)
App->>App: loadAutoRunDocuments(activeSessionId)
MarketplaceSheet->>App: onClose() [after 300ms animation]
App->>App: setShowMarketplaceSheet(false)
Reviews (1): Last reviewed commit: "fix(web): surface Browse Playbook Exchan..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/services/marketplace-service.ts`:
- Around line 451-469: The import path currently ignores a stale cache when
fetchManifest() fails, causing official playbooks to disappear; update the logic
in the block using readCache/isCacheValid/fetchManifest/writeCache so that on
fetchManifest() failure you fall back to the existing cache.manifest (even if
isCacheValid(cache) is false) and log that you are using a stale cache;
specifically, in the catch for fetchManifest() check if cache is present and set
officialManifest = cache.manifest before logging, otherwise keep the current
behavior of capturing the exception and continuing, ensuring mergeManifests
still receives the stale official manifest when available.
- Around line 491-625: The code currently proceeds even when importedDocs is
empty; add a guard after the document import loop (before building newPlaybook /
persisting) that checks if importedDocs.length === 0 and, if so, throws a
MarketplaceImportError with a clear message including marketplacePlaybook.title
so the import fails fast; use the existing MarketplaceImportError constructor
(same style as the read error) to abort before creating newPlaybook or writing
playbooksFilePath.
In `@src/main/web-server/handlers/messageHandlers.ts`:
- Around line 3957-3964: The catch block in messageHandlers.ts that currently
sends a typed failure for marketplace (the anonymous .catch handler that calls
this.send with type 'marketplace_get_manifest_result' and message.requestId)
must report unexpected exceptions to Sentry before replying; update those catch
blocks (also the similar ones around lines handling
marketplace_get_manifest_result, marketplace_list, marketplace_get_item, etc.)
to call reportHandlerError(err, { handler: 'marketplace_*', requestId:
message.requestId }) or captureException/captureMessage first, and only then
send the typed failure for known/recoverable errors (or re-throw for truly
unexpected ones); reuse the existing reportHandlerError utility or add a
typed-marketplace wrapper so every catch path records the error with context
before responding via this.send.
- Around line 3973-3980: Update isUntrustedLocalPath to reject dot-segments and
any backslash characters: ensure the function not only checks
absolute/tilde/Windows-drive prefixes but also returns true if playbookPath
contains backslashes or any segment equals '.' or '..' (e.g., split on '/' and
validate segments). Then, in each marketplace catch block that currently calls
sendMarketplaceFailure (the catches around lines handling marketplace import),
call captureException(err) or reportHandlerError(err) before invoking
sendMarketplaceFailure so exceptions are reported to Sentry; keep the existing
sendMarketplaceFailure behavior after reporting. Use the symbols
isUntrustedLocalPath, sendMarketplaceFailure, captureException, and
reportHandlerError to locate and change the code.
In `@src/main/web-server/web-server-factory.ts`:
- Around line 2595-2599: The catch block that handles marketplace import
failures (where err is normalized to errorMsg and logger.error is called for
playbookId) currently returns { success: false } without reporting to Sentry;
update that catch to call captureException(err, { tags: { area:
'marketplaceImport', playbookId } , level: 'error' }) from the Sentry utilities
(import captureException from src/utils/sentry.ts) before returning, so the
original Error object and context (playbookId) are sent to Sentry; keep the
existing logger.error and return value after the capture.
In `@src/web/mobile/MarketplaceSheet.tsx`:
- Around line 147-150: The catch blocks around the async marketplace loading
logic (the one that sets setManifestError when cancelled isn't true — and the
other similar catches at the noted ranges) are swallowing unexpected exceptions;
update those catch handlers to (1) distinguish expected/recoverable errors
(e.g., specific NETWORK_ERROR or known error types) and continue to set
setManifestError for those, (2) for any other unexpected error call the Sentry
helper (imported from src/utils/sentry.ts, e.g., captureException or
captureMessage) with contextual info (include which operation and any relevant
variables like cancelled or manifest id), and then re-throw the error so it
bubbles to global handlers; keep the existing local UI error behavior only for
handled/recoverable errors and do not suppress unexpected exceptions.
- Around line 219-227: The cancel-branch that handles filename === null
currently increments previewRequestIdRef and clears selection/content/loading
state but does not clear previewError or re-trigger the README fetch, leaving
the sheet stuck on an error/placeholder; update this branch to also clear
previewError (via its setter, e.g. setPreviewError(null)) and then invoke the
same README-loading routine used on mount/initial load (call the existing README
fetch function) so the README is re-fetched when returning from a doc preview
while keeping previewRequestIdRef, setSelectedDocFilename, setDocumentContent,
and setIsLoadingDocument behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df857283-0cc4-48fb-8e97-b7130b3af47d
📒 Files selected for processing (19)
src/__tests__/main/ipc/handlers/marketplace.test.tssrc/__tests__/main/web-server/handlers/messageHandlers.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/__tests__/web/mobile/App.test.tsxsrc/__tests__/web/mobile/AutoRunInline.test.tsxsrc/main/ipc/handlers/marketplace.tssrc/main/services/marketplace-service.tssrc/main/web-server/WebServer.tssrc/main/web-server/handlers/messageHandlers.tssrc/main/web-server/managers/CallbackRegistry.tssrc/main/web-server/types.tssrc/main/web-server/web-server-factory.tssrc/web/mobile/App.tsxsrc/web/mobile/AutoRunInline.tsxsrc/web/mobile/AutoRunPanel.tsxsrc/web/mobile/AutoRunSetupSheet.tsxsrc/web/mobile/MarketplaceSheet.tsxsrc/web/mobile/RightDrawer.tsxsrc/web/mobile/RightPanel.tsx
Comment on lines
+451
to
+469
| const cache = await readCache(app); | ||
| let officialManifest: MarketplaceManifest | null = null; | ||
| if (cache && isCacheValid(cache)) { | ||
| officialManifest = cache.manifest; | ||
| } else { | ||
| try { | ||
| officialManifest = await fetchManifest(); | ||
| await writeCache(app, officialManifest); | ||
| } catch (error) { | ||
| void captureException(error); | ||
| logger.warn( | ||
| 'Failed to fetch official manifest during import, continuing with local only', | ||
| LOG_CONTEXT, | ||
| { error } | ||
| ); | ||
| } | ||
| } | ||
| const localManifest = await readLocalManifest(app); | ||
| const manifest = mergeManifests(officialManifest, localManifest); |
There was a problem hiding this comment.
Import should fall back to stale cache the same way browsing does.
This path only uses the cached official manifest when it is still within TTL. Once the cache is stale, a transient fetchManifest() failure drops all official playbooks from import-time lookup, even though getMarketplaceManifest() still serves that same stale cache to the UI. The result is a visible playbook that import then claims does not exist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/services/marketplace-service.ts` around lines 451 - 469, The import
path currently ignores a stale cache when fetchManifest() fails, causing
official playbooks to disappear; update the logic in the block using
readCache/isCacheValid/fetchManifest/writeCache so that on fetchManifest()
failure you fall back to the existing cache.manifest (even if
isCacheValid(cache) is false) and log that you are using a stale cache;
specifically, in the catch for fetchManifest() check if cache is present and set
officialManifest = cache.manifest before logging, otherwise keep the current
behavior of capturing the exception and continuing, ensuring mergeManifests
still receives the stale official manifest when available.
Comment on lines
+491
to
+625
| const importedDocs: string[] = []; | ||
| for (const doc of marketplacePlaybook.documents) { | ||
| try { | ||
| const content = await fetchDocument(marketplacePlaybook.path, doc.filename); | ||
| const docPath = isRemote | ||
| ? `${targetPath}/${doc.filename}.md` | ||
| : path.join(targetPath, `${doc.filename}.md`); | ||
| if (isRemote) { | ||
| const writeResult = await writeFileRemote(docPath, content, sshConfig!); | ||
| if (!writeResult.success) { | ||
| throw new Error(writeResult.error || 'Failed to write remote file'); | ||
| } | ||
| } else { | ||
| await fs.writeFile(docPath, content, 'utf-8'); | ||
| } | ||
| importedDocs.push(doc.filename); | ||
| } catch (error) { | ||
| void captureException(error); | ||
| logger.warn(`Failed to import document ${doc.filename}`, LOG_CONTEXT, { error }); | ||
| } | ||
| } | ||
|
|
||
| // Build effective asset list (local: union manifest + discovered files) | ||
| const manifestAssets = marketplacePlaybook.assets ?? []; | ||
| let effectiveAssets = manifestAssets; | ||
| if (isLocalPath(marketplacePlaybook.path)) { | ||
| const discoveredAssets: string[] = []; | ||
| const resolvedPlaybookPath = resolveTildePath(marketplacePlaybook.path); | ||
| const localAssetsPath = path.join(resolvedPlaybookPath, 'assets'); | ||
| try { | ||
| const entries = await fs.readdir(localAssetsPath); | ||
| for (const entry of entries) { | ||
| const entryPath = path.join(localAssetsPath, entry); | ||
| try { | ||
| const stat = await fs.stat(entryPath); | ||
| if (stat.isFile()) discoveredAssets.push(entry); | ||
| } catch (error) { | ||
| void captureException(error); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { | ||
| logger.warn(`Failed to read local assets directory: ${localAssetsPath}`, LOG_CONTEXT, { | ||
| error, | ||
| }); | ||
| } | ||
| } | ||
| effectiveAssets = Array.from(new Set([...manifestAssets, ...discoveredAssets])); | ||
| } | ||
|
|
||
| const importedAssets: string[] = []; | ||
| if (effectiveAssets.length > 0) { | ||
| const assetsPath = isRemote ? `${targetPath}/assets` : path.join(targetPath, 'assets'); | ||
| if (isRemote) { | ||
| const mkdirResult = await mkdirRemote(assetsPath, sshConfig!, true); | ||
| if (!mkdirResult.success) { | ||
| logger.warn(`Failed to create remote assets directory: ${mkdirResult.error}`, LOG_CONTEXT); | ||
| } | ||
| } else { | ||
| await fs.mkdir(assetsPath, { recursive: true }); | ||
| } | ||
|
|
||
| for (const assetFilename of effectiveAssets) { | ||
| try { | ||
| const content = await fetchAsset(marketplacePlaybook.path, assetFilename); | ||
| const assetPath = isRemote | ||
| ? `${assetsPath}/${assetFilename}` | ||
| : path.join(assetsPath, assetFilename); | ||
| if (isRemote) { | ||
| const writeResult = await writeFileRemote(assetPath, content, sshConfig!); | ||
| if (!writeResult.success) { | ||
| throw new Error(writeResult.error || 'Failed to write remote asset file'); | ||
| } | ||
| } else { | ||
| await fs.writeFile(assetPath, content); | ||
| } | ||
| importedAssets.push(assetFilename); | ||
| } catch (error) { | ||
| void captureException(error); | ||
| logger.warn(`Failed to import asset ${assetFilename}`, LOG_CONTEXT, { error }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Persist only documents that actually wrote to disk so the playbook | ||
| // never references missing files. Filter manifest order against the | ||
| // importedDocs success set. | ||
| const importedDocSet = new Set(importedDocs); | ||
| const now = Date.now(); | ||
| const newPlaybook = { | ||
| id: crypto.randomUUID(), | ||
| name: marketplacePlaybook.title, | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| documents: marketplacePlaybook.documents | ||
| .filter((d) => importedDocSet.has(d.filename)) | ||
| .map((d) => ({ | ||
| filename: targetFolderName ? `${targetFolderName}/${d.filename}` : d.filename, | ||
| resetOnCompletion: d.resetOnCompletion, | ||
| })), | ||
| loopEnabled: marketplacePlaybook.loopEnabled, | ||
| maxLoops: marketplacePlaybook.maxLoops, | ||
| prompt: marketplacePlaybook.prompt ?? '', | ||
| }; | ||
|
|
||
| const playbooksDir = path.join(app.getPath('userData'), 'playbooks'); | ||
| await fs.mkdir(playbooksDir, { recursive: true }); | ||
| const playbooksFilePath = path.join(playbooksDir, `${sessionId}.json`); | ||
| let playbooks: any[] = []; | ||
| try { | ||
| const content = await fs.readFile(playbooksFilePath, 'utf-8'); | ||
| const data = JSON.parse(content); | ||
| playbooks = Array.isArray(data.playbooks) ? data.playbooks : []; | ||
| } catch (error) { | ||
| // ENOENT is normal (first save). Anything else (corrupt JSON, EACCES, | ||
| // etc.) means there's existing user data we couldn't read — refuse | ||
| // to silently overwrite it, since starting from [] would drop their | ||
| // previously-saved playbooks on the next write. | ||
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { | ||
| void captureException(error); | ||
| throw new MarketplaceImportError( | ||
| `Failed to read existing playbooks file (refusing to overwrite): ${error instanceof Error ? error.message : String(error)}`, | ||
| error | ||
| ); | ||
| } | ||
| } | ||
| playbooks.push(newPlaybook); | ||
| await fs.writeFile(playbooksFilePath, JSON.stringify({ playbooks }, null, 2), 'utf-8'); | ||
|
|
||
| logger.info( | ||
| `Successfully imported playbook "${marketplacePlaybook.title}" with ${importedDocs.length} documents and ${importedAssets.length} assets`, | ||
| LOG_CONTEXT | ||
| ); | ||
|
|
||
| return { playbook: newPlaybook, importedDocs, importedAssets }; |
There was a problem hiding this comment.
Fail the import when no documents were written.
The per-document loop is intentionally tolerant, but if every fetch/write fails we still create and persist a playbook with documents: [] and return success. That closes the marketplace sheet and leaves the user with an unusable imported playbook.
💡 Minimal guard
const importedDocs: string[] = [];
for (const doc of marketplacePlaybook.documents) {
try {
const content = await fetchDocument(marketplacePlaybook.path, doc.filename);
const docPath = isRemote
? `${targetPath}/${doc.filename}.md`
: path.join(targetPath, `${doc.filename}.md`);
if (isRemote) {
const writeResult = await writeFileRemote(docPath, content, sshConfig!);
if (!writeResult.success) {
throw new Error(writeResult.error || 'Failed to write remote file');
}
} else {
await fs.writeFile(docPath, content, 'utf-8');
}
importedDocs.push(doc.filename);
} catch (error) {
void captureException(error);
logger.warn(`Failed to import document ${doc.filename}`, LOG_CONTEXT, { error });
}
}
+
+ if (importedDocs.length === 0) {
+ throw new MarketplaceImportError(
+ `Failed to import any documents for playbook: ${marketplacePlaybook.id}`
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const importedDocs: string[] = []; | |
| for (const doc of marketplacePlaybook.documents) { | |
| try { | |
| const content = await fetchDocument(marketplacePlaybook.path, doc.filename); | |
| const docPath = isRemote | |
| ? `${targetPath}/${doc.filename}.md` | |
| : path.join(targetPath, `${doc.filename}.md`); | |
| if (isRemote) { | |
| const writeResult = await writeFileRemote(docPath, content, sshConfig!); | |
| if (!writeResult.success) { | |
| throw new Error(writeResult.error || 'Failed to write remote file'); | |
| } | |
| } else { | |
| await fs.writeFile(docPath, content, 'utf-8'); | |
| } | |
| importedDocs.push(doc.filename); | |
| } catch (error) { | |
| void captureException(error); | |
| logger.warn(`Failed to import document ${doc.filename}`, LOG_CONTEXT, { error }); | |
| } | |
| } | |
| // Build effective asset list (local: union manifest + discovered files) | |
| const manifestAssets = marketplacePlaybook.assets ?? []; | |
| let effectiveAssets = manifestAssets; | |
| if (isLocalPath(marketplacePlaybook.path)) { | |
| const discoveredAssets: string[] = []; | |
| const resolvedPlaybookPath = resolveTildePath(marketplacePlaybook.path); | |
| const localAssetsPath = path.join(resolvedPlaybookPath, 'assets'); | |
| try { | |
| const entries = await fs.readdir(localAssetsPath); | |
| for (const entry of entries) { | |
| const entryPath = path.join(localAssetsPath, entry); | |
| try { | |
| const stat = await fs.stat(entryPath); | |
| if (stat.isFile()) discoveredAssets.push(entry); | |
| } catch (error) { | |
| void captureException(error); | |
| } | |
| } | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| logger.warn(`Failed to read local assets directory: ${localAssetsPath}`, LOG_CONTEXT, { | |
| error, | |
| }); | |
| } | |
| } | |
| effectiveAssets = Array.from(new Set([...manifestAssets, ...discoveredAssets])); | |
| } | |
| const importedAssets: string[] = []; | |
| if (effectiveAssets.length > 0) { | |
| const assetsPath = isRemote ? `${targetPath}/assets` : path.join(targetPath, 'assets'); | |
| if (isRemote) { | |
| const mkdirResult = await mkdirRemote(assetsPath, sshConfig!, true); | |
| if (!mkdirResult.success) { | |
| logger.warn(`Failed to create remote assets directory: ${mkdirResult.error}`, LOG_CONTEXT); | |
| } | |
| } else { | |
| await fs.mkdir(assetsPath, { recursive: true }); | |
| } | |
| for (const assetFilename of effectiveAssets) { | |
| try { | |
| const content = await fetchAsset(marketplacePlaybook.path, assetFilename); | |
| const assetPath = isRemote | |
| ? `${assetsPath}/${assetFilename}` | |
| : path.join(assetsPath, assetFilename); | |
| if (isRemote) { | |
| const writeResult = await writeFileRemote(assetPath, content, sshConfig!); | |
| if (!writeResult.success) { | |
| throw new Error(writeResult.error || 'Failed to write remote asset file'); | |
| } | |
| } else { | |
| await fs.writeFile(assetPath, content); | |
| } | |
| importedAssets.push(assetFilename); | |
| } catch (error) { | |
| void captureException(error); | |
| logger.warn(`Failed to import asset ${assetFilename}`, LOG_CONTEXT, { error }); | |
| } | |
| } | |
| } | |
| // Persist only documents that actually wrote to disk so the playbook | |
| // never references missing files. Filter manifest order against the | |
| // importedDocs success set. | |
| const importedDocSet = new Set(importedDocs); | |
| const now = Date.now(); | |
| const newPlaybook = { | |
| id: crypto.randomUUID(), | |
| name: marketplacePlaybook.title, | |
| createdAt: now, | |
| updatedAt: now, | |
| documents: marketplacePlaybook.documents | |
| .filter((d) => importedDocSet.has(d.filename)) | |
| .map((d) => ({ | |
| filename: targetFolderName ? `${targetFolderName}/${d.filename}` : d.filename, | |
| resetOnCompletion: d.resetOnCompletion, | |
| })), | |
| loopEnabled: marketplacePlaybook.loopEnabled, | |
| maxLoops: marketplacePlaybook.maxLoops, | |
| prompt: marketplacePlaybook.prompt ?? '', | |
| }; | |
| const playbooksDir = path.join(app.getPath('userData'), 'playbooks'); | |
| await fs.mkdir(playbooksDir, { recursive: true }); | |
| const playbooksFilePath = path.join(playbooksDir, `${sessionId}.json`); | |
| let playbooks: any[] = []; | |
| try { | |
| const content = await fs.readFile(playbooksFilePath, 'utf-8'); | |
| const data = JSON.parse(content); | |
| playbooks = Array.isArray(data.playbooks) ? data.playbooks : []; | |
| } catch (error) { | |
| // ENOENT is normal (first save). Anything else (corrupt JSON, EACCES, | |
| // etc.) means there's existing user data we couldn't read — refuse | |
| // to silently overwrite it, since starting from [] would drop their | |
| // previously-saved playbooks on the next write. | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| void captureException(error); | |
| throw new MarketplaceImportError( | |
| `Failed to read existing playbooks file (refusing to overwrite): ${error instanceof Error ? error.message : String(error)}`, | |
| error | |
| ); | |
| } | |
| } | |
| playbooks.push(newPlaybook); | |
| await fs.writeFile(playbooksFilePath, JSON.stringify({ playbooks }, null, 2), 'utf-8'); | |
| logger.info( | |
| `Successfully imported playbook "${marketplacePlaybook.title}" with ${importedDocs.length} documents and ${importedAssets.length} assets`, | |
| LOG_CONTEXT | |
| ); | |
| return { playbook: newPlaybook, importedDocs, importedAssets }; | |
| const importedDocs: string[] = []; | |
| for (const doc of marketplacePlaybook.documents) { | |
| try { | |
| const content = await fetchDocument(marketplacePlaybook.path, doc.filename); | |
| const docPath = isRemote | |
| ? `${targetPath}/${doc.filename}.md` | |
| : path.join(targetPath, `${doc.filename}.md`); | |
| if (isRemote) { | |
| const writeResult = await writeFileRemote(docPath, content, sshConfig!); | |
| if (!writeResult.success) { | |
| throw new Error(writeResult.error || 'Failed to write remote file'); | |
| } | |
| } else { | |
| await fs.writeFile(docPath, content, 'utf-8'); | |
| } | |
| importedDocs.push(doc.filename); | |
| } catch (error) { | |
| void captureException(error); | |
| logger.warn(`Failed to import document ${doc.filename}`, LOG_CONTEXT, { error }); | |
| } | |
| } | |
| if (importedDocs.length === 0) { | |
| throw new MarketplaceImportError( | |
| `Failed to import any documents for playbook: ${marketplacePlaybook.id}` | |
| ); | |
| } | |
| // Build effective asset list (local: union manifest + discovered files) | |
| const manifestAssets = marketplacePlaybook.assets ?? []; | |
| let effectiveAssets = manifestAssets; | |
| if (isLocalPath(marketplacePlaybook.path)) { | |
| const discoveredAssets: string[] = []; | |
| const resolvedPlaybookPath = resolveTildePath(marketplacePlaybook.path); | |
| const localAssetsPath = path.join(resolvedPlaybookPath, 'assets'); | |
| try { | |
| const entries = await fs.readdir(localAssetsPath); | |
| for (const entry of entries) { | |
| const entryPath = path.join(localAssetsPath, entry); | |
| try { | |
| const stat = await fs.stat(entryPath); | |
| if (stat.isFile()) discoveredAssets.push(entry); | |
| } catch (error) { | |
| void captureException(error); | |
| } | |
| } | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| logger.warn(`Failed to read local assets directory: ${localAssetsPath}`, LOG_CONTEXT, { | |
| error, | |
| }); | |
| } | |
| } | |
| effectiveAssets = Array.from(new Set([...manifestAssets, ...discoveredAssets])); | |
| } | |
| const importedAssets: string[] = []; | |
| if (effectiveAssets.length > 0) { | |
| const assetsPath = isRemote ? `${targetPath}/assets` : path.join(targetPath, 'assets'); | |
| if (isRemote) { | |
| const mkdirResult = await mkdirRemote(assetsPath, sshConfig!, true); | |
| if (!mkdirResult.success) { | |
| logger.warn(`Failed to create remote assets directory: ${mkdirResult.error}`, LOG_CONTEXT); | |
| } | |
| } else { | |
| await fs.mkdir(assetsPath, { recursive: true }); | |
| } | |
| for (const assetFilename of effectiveAssets) { | |
| try { | |
| const content = await fetchAsset(marketplacePlaybook.path, assetFilename); | |
| const assetPath = isRemote | |
| ? `${assetsPath}/${assetFilename}` | |
| : path.join(assetsPath, assetFilename); | |
| if (isRemote) { | |
| const writeResult = await writeFileRemote(assetPath, content, sshConfig!); | |
| if (!writeResult.success) { | |
| throw new Error(writeResult.error || 'Failed to write remote asset file'); | |
| } | |
| } else { | |
| await fs.writeFile(assetPath, content); | |
| } | |
| importedAssets.push(assetFilename); | |
| } catch (error) { | |
| void captureException(error); | |
| logger.warn(`Failed to import asset ${assetFilename}`, LOG_CONTEXT, { error }); | |
| } | |
| } | |
| } | |
| // Persist only documents that actually wrote to disk so the playbook | |
| // never references missing files. Filter manifest order against the | |
| // importedDocs success set. | |
| const importedDocSet = new Set(importedDocs); | |
| const now = Date.now(); | |
| const newPlaybook = { | |
| id: crypto.randomUUID(), | |
| name: marketplacePlaybook.title, | |
| createdAt: now, | |
| updatedAt: now, | |
| documents: marketplacePlaybook.documents | |
| .filter((d) => importedDocSet.has(d.filename)) | |
| .map((d) => ({ | |
| filename: targetFolderName ? `${targetFolderName}/${d.filename}` : d.filename, | |
| resetOnCompletion: d.resetOnCompletion, | |
| })), | |
| loopEnabled: marketplacePlaybook.loopEnabled, | |
| maxLoops: marketplacePlaybook.maxLoops, | |
| prompt: marketplacePlaybook.prompt ?? '', | |
| }; | |
| const playbooksDir = path.join(app.getPath('userData'), 'playbooks'); | |
| await fs.mkdir(playbooksDir, { recursive: true }); | |
| const playbooksFilePath = path.join(playbooksDir, `${sessionId}.json`); | |
| let playbooks: any[] = []; | |
| try { | |
| const content = await fs.readFile(playbooksFilePath, 'utf-8'); | |
| const data = JSON.parse(content); | |
| playbooks = Array.isArray(data.playbooks) ? data.playbooks : []; | |
| } catch (error) { | |
| // ENOENT is normal (first save). Anything else (corrupt JSON, EACCES, | |
| // etc.) means there's existing user data we couldn't read — refuse | |
| // to silently overwrite it, since starting from [] would drop their | |
| // previously-saved playbooks on the next write. | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| void captureException(error); | |
| throw new MarketplaceImportError( | |
| `Failed to read existing playbooks file (refusing to overwrite): ${error instanceof Error ? error.message : String(error)}`, | |
| error | |
| ); | |
| } | |
| } | |
| playbooks.push(newPlaybook); | |
| await fs.writeFile(playbooksFilePath, JSON.stringify({ playbooks }, null, 2), 'utf-8'); | |
| logger.info( | |
| `Successfully imported playbook "${marketplacePlaybook.title}" with ${importedDocs.length} documents and ${importedAssets.length} assets`, | |
| LOG_CONTEXT | |
| ); | |
| return { playbook: newPlaybook, importedDocs, importedAssets }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/services/marketplace-service.ts` around lines 491 - 625, The code
currently proceeds even when importedDocs is empty; add a guard after the
document import loop (before building newPlaybook / persisting) that checks if
importedDocs.length === 0 and, if so, throws a MarketplaceImportError with a
clear message including marketplacePlaybook.title so the import fails fast; use
the existing MarketplaceImportError constructor (same style as the read error)
to abort before creating newPlaybook or writing playbooksFilePath.
Comment on lines
+3957
to
+3964
| .catch((error) => { | ||
| this.send(client, { | ||
| type: 'marketplace_get_manifest_result', | ||
| success: false, | ||
| error: `Failed to load marketplace: ${error.message}`, | ||
| requestId: message.requestId, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Capture marketplace handler exceptions before sending typed failures.
These catch blocks turn every callback rejection into a typed response, but they never report unexpected exceptions to Sentry. That makes real production faults invisible while still hiding them from the caller. Reuse reportHandlerError or add a typed-marketplace variant that calls captureException first.
As per coding guidelines, "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly (e.g., NETWORK_ERROR). For unexpected errors, re-throw them to allow Sentry to capture them. Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context."
Also applies to: 4081-4088, 4144-4151, 4241-4249
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers.ts` around lines 3957 - 3964,
The catch block in messageHandlers.ts that currently sends a typed failure for
marketplace (the anonymous .catch handler that calls this.send with type
'marketplace_get_manifest_result' and message.requestId) must report unexpected
exceptions to Sentry before replying; update those catch blocks (also the
similar ones around lines handling marketplace_get_manifest_result,
marketplace_list, marketplace_get_item, etc.) to call reportHandlerError(err, {
handler: 'marketplace_*', requestId: message.requestId }) or
captureException/captureMessage first, and only then send the typed failure for
known/recoverable errors (or re-throw for truly unexpected ones); reuse the
existing reportHandlerError utility or add a typed-marketplace wrapper so every
catch path records the error with context before responding via this.send.
Comment on lines
+3973
to
+3980
| private isUntrustedLocalPath(playbookPath: string): boolean { | ||
| return ( | ||
| playbookPath.startsWith('/') || | ||
| playbookPath.startsWith('\\') || | ||
| playbookPath.startsWith('~/') || | ||
| playbookPath.startsWith('~\\') || | ||
| /^[a-zA-Z]:[\\/]/.test(playbookPath) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -p 'marketplace-service.ts' src/main | while read -r file; do
echo "===== $file ====="
rg -n -C4 'getMarketplace(Document|Readme)|playbookPath|resolve\(|join\(|readFile\(' "$file"
doneRepository: RunMaestro/Maestro
Length of output: 8092
🏁 Script executed:
wc -l src/main/web-server/handlers/messageHandlers.tsRepository: RunMaestro/Maestro
Length of output: 115
🏁 Script executed:
# Check the area around isUntrustedLocalPath usage (3973-3980) and surrounding error handlers
sed -n '3950,3985p' src/main/web-server/handlers/messageHandlers.ts | cat -nRepository: RunMaestro/Maestro
Length of output: 1410
🏁 Script executed:
# Check the catch blocks mentioned: 3957-3964, 4081-4088, 4144-4151, 4241-4249
sed -n '3957,3965p' src/main/web-server/handlers/messageHandlers.ts | cat -n
sed -n '4081,4089p' src/main/web-server/handlers/messageHandlers.ts | cat -n
sed -n '4144,4152p' src/main/web-server/handlers/messageHandlers.ts | cat -n
sed -n '4241,4249p' src/main/web-server/handlers/messageHandlers.ts | cat -nRepository: RunMaestro/Maestro
Length of output: 1116
🏁 Script executed:
# Check if reportHandlerError is used elsewhere in the file for Sentry reporting
rg -n 'reportHandlerError|captureException' src/main/web-server/handlers/messageHandlers.ts | head -20Repository: RunMaestro/Maestro
Length of output: 541
Fix two issues: harden playbookPath validation and capture exceptions in marketplace catch blocks.
-
Path traversal: The helper still allows
../foo,./foo, and backslashes through. Reject dot segments (..,.) and backslashes inplaybookPath. Downstream validation has some protection, but defensive validation at the entry point prevents future bugs if the code changes. -
Silent error swallowing: The four catch blocks at lines 3957–3964, 4081–4088, 4144–4151, and 4241–4249 call
sendMarketplaceFailure()without reporting exceptions to Sentry. This violates the repository guideline: "Do not silently swallow errors." UsecaptureException()orreportHandlerError()in each catch block, like the existing handlers elsewhere in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers.ts` around lines 3973 - 3980,
Update isUntrustedLocalPath to reject dot-segments and any backslash characters:
ensure the function not only checks absolute/tilde/Windows-drive prefixes but
also returns true if playbookPath contains backslashes or any segment equals '.'
or '..' (e.g., split on '/' and validate segments). Then, in each marketplace
catch block that currently calls sendMarketplaceFailure (the catches around
lines handling marketplace import), call captureException(err) or
reportHandlerError(err) before invoking sendMarketplaceFailure so exceptions are
reported to Sentry; keep the existing sendMarketplaceFailure behavior after
reporting. Use the symbols isUntrustedLocalPath, sendMarketplaceFailure,
captureException, and reportHandlerError to locate and change the code.
Comment on lines
+2595
to
+2599
| } catch (err) { | ||
| const errorMsg = err instanceof Error ? err.message : String(err); | ||
| logger.error(`Marketplace import failed for ${playbookId}: ${errorMsg}`, 'WebServer'); | ||
| return { success: false, error: errorMsg }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Capture unexpected marketplace import exceptions in Sentry before returning.
On Line 2595-Line 2599, exceptions are converted to { success: false } and only logged, so production error tracking can miss failures. Add captureException with context in this catch path.
Proposed fix
import { logger } from '../utils/logger';
+import { captureException } from '../utils/sentry';
@@
} catch (err) {
const errorMsg = err instanceof Error ? err.message : String(err);
+ captureException(err, {
+ operation: 'webServerFactory:importMarketplacePlaybook',
+ sessionId,
+ playbookId,
+ targetFolderName,
+ });
logger.error(`Marketplace import failed for ${playbookId}: ${errorMsg}`, 'WebServer');
return { success: false, error: errorMsg };
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/web-server-factory.ts` around lines 2595 - 2599, The
catch block that handles marketplace import failures (where err is normalized to
errorMsg and logger.error is called for playbookId) currently returns { success:
false } without reporting to Sentry; update that catch to call
captureException(err, { tags: { area: 'marketplaceImport', playbookId } , level:
'error' }) from the Sentry utilities (import captureException from
src/utils/sentry.ts) before returning, so the original Error object and context
(playbookId) are sent to Sentry; keep the existing logger.error and return value
after the capture.
Comment on lines
+147
to
+150
| } catch (err) { | ||
| if (cancelled) return; | ||
| setManifestError(err instanceof Error ? err.message : 'Failed to load marketplace data'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Report unexpected async failures instead of only showing inline UI errors.
These catches turn every thrown exception into local component state and return. That preserves the recoverable UX path, but unexpected failures here never reach Sentry, so production breakages in the new marketplace flow become invisible.
As per coding guidelines, "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly (e.g., NETWORK_ERROR). For unexpected errors, re-throw them to allow Sentry to capture them. Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context."
Also applies to: 206-210, 245-249, 278-281
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/web/mobile/MarketplaceSheet.tsx` around lines 147 - 150, The catch blocks
around the async marketplace loading logic (the one that sets setManifestError
when cancelled isn't true — and the other similar catches at the noted ranges)
are swallowing unexpected exceptions; update those catch handlers to (1)
distinguish expected/recoverable errors (e.g., specific NETWORK_ERROR or known
error types) and continue to set setManifestError for those, (2) for any other
unexpected error call the Sentry helper (imported from src/utils/sentry.ts,
e.g., captureException or captureMessage) with contextual info (include which
operation and any relevant variables like cancelled or manifest id), and then
re-throw the error so it bubbles to global handlers; keep the existing local UI
error behavior only for handled/recoverable errors and do not suppress
unexpected exceptions.
Comment on lines
+219
to
+227
| if (filename === null) { | ||
| // Cancel any in-flight document fetch and immediately surface | ||
| // the README — bumping the request id ensures a slow document | ||
| // response can't overwrite this state later. | ||
| ++previewRequestIdRef.current; | ||
| setSelectedDocFilename(null); | ||
| setDocumentContent(null); | ||
| setIsLoadingDocument(false); | ||
| return; |
There was a problem hiding this comment.
Re-load README when switching back from a document preview.
This branch cancels the in-flight document request, but it never clears previewError or re-fetches the README. If the user taps a doc before the initial README load finishes, then goes back to README.md, the sheet can stay stuck on the old error / “No README available” placeholder even when a README exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/web/mobile/MarketplaceSheet.tsx` around lines 219 - 227, The
cancel-branch that handles filename === null currently increments
previewRequestIdRef and clears selection/content/loading state but does not
clear previewError or re-trigger the README fetch, leaving the sheet stuck on an
error/placeholder; update this branch to also clear previewError (via its
setter, e.g. setPreviewError(null)) and then invoke the same README-loading
routine used on mount/initial load (call the existing README fetch function) so
the README is re-fetched when returning from a doc preview while keeping
previewRequestIdRef, setSelectedDocFilename, setDocumentContent, and
setIsLoadingDocument behavior intact.
Collaborator
|
Hey @chr1syy — thanks so much for the follow-up to close Gap 3! 🙏 The "Browse Playbook Exchange" CTA in the AutoRun empty state is a clean parity fix and the opt-in prop pattern keeps existing callers unaffected. Heads up though: GitHub is reporting merge conflicts against Thanks again! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #947 (
feat(web): expose Playbook Exchange to mobile/web) closing Gap 3 from the post-soak parity gist.Desktop has an "Add Docs" affordance that surfaces both Create document and Browse Marketplace as co-equal entry points. On mobile/web, the empty state of
AutoRunInlineonly offered "+ Create document" — users with a fresh AutoRun folder had no in-UI path to discover existing playbooks and had to know to open the launch sheet just to find the marketplace entry.This PR adds a "Browse Playbook Exchange" CTA next to "+ Create document" in the empty state, matching desktop's Docs Overview behaviour. It reuses the
MarketplaceSheetand WS infrastructure introduced by #947 — no new IPC/server surface.Stacked on #947
This branch is stacked on top of
feat/web-playbook-exchange, so the diff currently includes #947's commits. Once #947 lands inrc, this PR's diff collapses to just the Gap 3 commit (5b39a3c). The single new commit on top of #947 is what's actually being added here.Changes (this PR's commit only)
src/web/mobile/AutoRunInline.tsx: optionalonOpenMarketplaceprop; second CTA rendered in the empty state when provided.src/web/mobile/AutoRunPanel.tsx,RightDrawer.tsx(AutoRunTabContent),RightPanel.tsx: prop plumbed through.src/web/mobile/App.tsx:handleOpenMarketplaceSheetwired into bothAutoRunPanel(full-screen overlay) andRightPanel(inline tab).src/__tests__/web/mobile/AutoRunInline.test.tsx: 4 new scoped tests covering CTA visibility, hidden-when-prop-omitted, render-when-provided, and onClick.The CTA is opt-in (prop is optional), so existing callers and tests keep working.
Test plan
MarketplaceSheetopens.onOpenMarketplace(regression guard).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests