feat(browser): record and replay reusable workflows - #2333
Conversation
f0515f1 to
859d8d0
Compare
|
Slice 1 is ready for maintainer review. I consolidated the final fixes into commit |
|
Thanks for building this on the existing browser page, visibility lease, CDP cancellation, and storage paths. I found one security issue and several workflow gaps in the current head. P1: Tokens and passwords in URLs are persisted verbatimThe recorder accepts every HTTP or HTTPS URL as a Recording either of these will put the secret in
The existing sensitive-field filtering only covers form values. URL userinfo, sensitive query parameters, and fragments need equivalent handling before an action is created. Rejecting the save with a clear explanation would be safer than persisting the URL. P2: Multiple sensitive inputs cannot be distinguishedEvery redacted action is rendered with the same runtime-value label: A password-change flow with old password, new password, confirmation, and OTP produces several identical fields. The user has no reliable way to map a value to its step. Each field needs a unique, understandable label, such as the step number plus a safe locator description. Its accessible name should be unique as well. P2: Saved workflows cannot be inspected before runningThe workflow list shows only the name, update time, and action count: There is no way to inspect the saved URLs, targets, or waits before execution. This is particularly risky after sensitive values have been removed. The recorder already has an action preview, so reusing that view would avoid a second representation. P2: Delete is immediate and irreversibleThe delete handler writes the new store state immediately, with no confirmation or undo: These workflows may be expensive to recreate. A confirmation, short undo window, or recoverable delete would make the operation reversible. P3: Workflows over 500 steps are silently truncatedOnce the recorder reaches 500 actions, later actions are ignored and the first 500 are still saved as a successful workflow: This creates a workflow that looks valid but cannot reproduce the recorded task. Reaching the limit should stop recording with a visible error or mark the draft as unsavable. P3: Discarded recording drafts remain in memoryStopped drafts are stored in an unbounded Starting another recording drops the renderer's reference to the old draft, so it can no longer be saved or released. Keeping at most one pending draft per session, or adding an explicit discard/TTL, would remove that unbounded state. GitHub currently marks the PR as conflicting. Current |
0aa851e to
1bd504f
Compare
|
Updated in |
1bd504f to
5150d5f
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the thorough revision. The workflow preview, runtime-value labels, delete confirmation, explicit action limit, and normal draft cleanup all look much clearer. I did not find a P0 or P1 issue, and I’m approving this revision.
Two non-blocking lifecycle findings remain:
P2 — A recording can outlive its browser session
Leaving the Sessions surface can leave the main-process recording active after its controls disappear. More importantly, archiving or deleting the session disposes its Browser view before the workflow service is released; the recording timer or a later cleanup can then call getOrCreate and recreate the deleted view. prepare-view can similarly replace the visible session without releasing the previous recording.
The clean follow-up would be one idempotent BrowserWorkflowService.releaseSession(sessionId) operation that cancels recordings and runs, clears timers/transitions and drafts, and only accesses an existing view during cleanup—never creates one. Panel teardown, visibility changes, prepare-view, and session retirement should share that operation.
P3 — A late stop result can leave an orphan draft
If the panel unmounts before stopRecording() returns, cleanup does not yet know the new draftId; the draft can then be created without an owner. The same session-level release operation, plus a deferred stop/cancel regression test, would close this race.
I’m not treating incomplete credential-name detection as a blocking issue. A finite URL/DOM denylist cannot prove that arbitrary page data is non-sensitive, and these workflows remain local. The sustainable future option would be allowing users to mark any recorded value as “ask at run time,” rather than continually expanding the regex.
GitHub currently reports the PR as conflicting, so it will still need to be rebased onto current main and have CI rerun before merge. The overall Slice 1 boundary remains coherent; I do not think it needs to be split further.
中文对照
感谢这轮认真调整。流程预览、运行时敏感值提示、删除确认、步骤上限和普通草稿回收都清楚了很多。目前没有发现 P0/P1 问题，我会 Approve。
还有两个不阻塞合并的生命周期问题：
P2 — 录制状态可能脱离 browser session 继续存活
离开会话页面后，main 侧录制可能还在运行，但 UI 已经没有停止入口。更具体的是，归档或删除 session 会先销毁 Browser view，而 workflow service 尚未释放；录制定时器或后续 cleanup 再调用 getOrCreate 时，可能把已经删除的 view 重新创建。prepare-view 切换可见 session 时也有类似缺口。
后续最干净的修法是让 BrowserWorkflowService 提供一个幂等的 releaseSession(sessionId)：统一取消 recording/run，清理 timer、transition 和 draft；清理时只读取已有 view，绝不新建。页面卸载、可见会话切换、prepare-view 和 session retirement 都复用这一入口。
P3 — Stop 返回较晚时可能留下无人持有的草稿
如果组件先卸载、stopRecording() 后返回，cleanup 当时还不知道新的 draftId，草稿可能留在 main 内存中。上面的 session-level release，加一条延迟 stop/cancel 的竞态测试，就能一起收口。
我不再把凭据名称识别不完整视为阻塞问题。有限的 URL/DOM denylist 本来就无法证明网页数据一定不敏感，而且 workflow 只保存在本地。更可持续的后续方案，是允许用户把任意录制值标记成“运行时询问”，而不是不断扩充正则。
当前 PR 与最新 main 仍有冲突，合并前需要 rebase 并重跑 CI。整体 Slice 1 仍是一条完整的垂直切片，不需要继续拆分。
714c669 to
1b51e88
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Codex automated review
I reviewed exact head 1b51e881817b891914470479c3b97384cf1ee403 against #1557 and the prior review ledger, tracing contract/storage, recorder injection, WebContents event ordering, runner cancellation/release, IPC/UI, and the E2E harness. The previous lifecycle findings are addressed on this head and all checks are green. Two current safety/replay issues remain; see the P2 inline findings.
The browser-workflow production path is one coherent vertical slice and should not be mechanically split despite its size. One unrelated test intent is mixed in, however: the fake-backend hold-open fixture plus staged slash-command E2E does not depend on browser workflows and should move to its own small PR/commit. I did not find a browser-workflow test block that can be safely deleted.
Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.
1b51e88 to
e16bda4
Compare
|
Thanks, LGTM except two thing:
|
e16bda4 to
4031068
Compare
4031068 to
520d327
Compare
📝 WalkthroughProblem solvedThis PR adds reusable browser workflows for human recording and deterministic replay. It provides:
Agent parity remains deferred to Slice 2. Source of truthThe PR extends the existing browser, preload, renderer, core, and storage layers. It does not create an isolated parallel workflow path. The shared The implementation adds a new browser-workflow feature path, but it uses existing browser sessions, Scope and complexityThe scope is coherent for Slice 1 because recording, validation, redaction, persistence, replay, cancellation, and lifecycle handling must work together. The added complexity is supported by the required behavior:
The main-process service centralizes these concerns instead of duplicating them across IPC handlers and UI components. The diff includes extensive regression coverage. Any test helper or overlapping implementation can be simplified only after confirming that it still covers persistence, redaction, navigation ordering, cancellation, concurrency, lifecycle cleanup, input types, and replay failures. No specific deletion is safe based on the provided summary alone. Validation and risksTests cover:
The author reported successful local validation and green GitHub checks for the latest revision. Required checks remain unverified here because direct check results were not provided. The review summary still identifies unresolved concerns about distinct labels for multiple sensitive inputs, pre-run workflow inspection, irreversible deletion, the 500-step limit behavior, and discarded draft retention. The reported conflict/rebase status and the request for screenshots also require confirmation against the current branch. Review-relevant risks
WalkthroughAdds browser workflow recording, validation, persistence, replay, cancellation, sensitive-value handling, progress reporting, and desktop UI integration. The change includes unit, lifecycle, storage, and end-to-end tests. ChangesBrowser workflow foundation
Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: 🟡 Moderate · up to Workflow recording may fail because of an incompatible browser event callback, while switching workflows can leave the previous session active; several cleanup and readiness failures can also produce unhandled errors, and long workflows may hold browser access for hours. These are concrete runtime and availability risks that should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant User
participant BrowserPanel
participant preload
participant browser-ipc-main
participant browser-workflow-service
participant BrowserViewController
participant BrowserWorkflowStore
User->>BrowserPanel: start recording
BrowserPanel->>preload: startRecording(sessionId)
preload->>browser-ipc-main: invoke recording IPC
browser-ipc-main->>browser-workflow-service: start recording
browser-workflow-service->>BrowserViewController: install recorder
BrowserViewController-->>browser-workflow-service: recorder events and navigation
User->>BrowserPanel: save draft
BrowserPanel->>preload: saveRecording(workflow)
preload->>browser-ipc-main: invoke save IPC
browser-ipc-main->>BrowserWorkflowStore: persist validated workflow
User->>BrowserPanel: run workflow
BrowserPanel->>preload: run(workflowId, sensitiveValues)
preload->>browser-ipc-main: invoke run IPC
browser-ipc-main->>browser-workflow-service: replay workflow
browser-workflow-service-->>BrowserPanel: progress events
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (8)
packages/storage/src/browser-workflow-store.ts (1)
53-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the duplicate validation pass.
isBrowserWorkflowFileat Line 53 already runsisBrowserWorkflowon every entry. Line 58 then revalidates the same objects and returns them unchanged. Return the array directly.♻️ Proposed simplification
- return parsed.workflows.map((workflow) => validateBrowserWorkflow(workflow)); + return parsed.workflows;Then remove
validateBrowserWorkflowfrom the import at Line 3-7 ifsaveis the only remaining user (it still needs it).Source: Path instructions
apps/desktop/src/main/__tests__/browser-workflow.test.ts (4)
410-413: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis assertion cannot fail.
recorderis the object assigned towindow.__makaBrowserWorkflowRecorderV1. It holdscredential,drain, anddispose. The recorded events live in a closure variable, and functions are dropped byJSON.stringify. The serialized value is therefore always{"credential":"test-credential"}, and the regex never matches, even if the recorder did capturefallback-secret.Line 411 already proves the behavior. Delete Line 412, or assert against the drained events instead.
♻️ Proposed change
- assert.deepEqual(recorder.drain(), []); - assert.doesNotMatch(JSON.stringify(recorder), /fallback-secret/); + assert.deepEqual(recorder.drain(), []);Source: Path instructions
1739-1745: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the optional-access guard for
releaseSession.Other tests in this file call
service.releaseSession(...)directly (Lines 1611, 1654, 2405, 2446). The cast and theif (!releaseSession)branch here are unreachable in practice and add a second, weaker contract for the same method.♻️ Proposed simplification
- const releaseSession = (service as unknown as { releaseSession?: (sessionId: string) => Promise<void> }) - .releaseSession; - if (!releaseSession) { - drainRelease.resolve([]); - await stopping; - assert.fail('BrowserWorkflowService.releaseSession is required'); - } - const releasing = releaseSession.call(service, 'session-1'); + const releasing = service.releaseSession('session-1');Source: Path instructions
64-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe double cast weakens this assertion.
getBrowserWorkflowCopy('en') as unknown as Record<string, unknown>bypasses the copy type. IfwaitNavigationActionis missing from the declared copy type, this test still passes and the type drift stays hidden. Read the property through the real type instead.♻️ Proposed change
- const copy = getBrowserWorkflowCopy('en') as unknown as Record<string, unknown>; - assert.equal(copy.waitNavigationAction, 'Wait for page navigation'); + assert.equal(getBrowserWorkflowCopy('en').waitNavigationAction, 'Wait for page navigation');Source: Path instructions
599-626: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLine 625 asserts a polling implementation detail.
assert.equal(reads, 2)fixes the exact number ofevaluatecalls inwaitForWorkflowUrl. Any change to the poll loop breaks this test without a behavior change. Line 603 already guarantees that no second navigation is issued.Source: Path instructions
apps/desktop/src/main/browser/browser-workflow-service.ts (2)
576-582: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the per-action wait instead of the aggregate lease.
timeoutMssums a worst-case budget for every action and applies the sum to onerunWithPagecall. A workflow at the 500-action limit produces a lease timeout of several hours. One hung action then consumes the whole aggregate budget, and themutatelease stays held for that period. Other browser automation on the session is blocked until the user cancels.Cap the aggregate value, or pass a per-action timeout to
runBrowserWorkflowActionso a single stuck action fails fast.♻️ Cap the aggregate lease
const timeoutMs = Math.max( 25_000, - workflow.actions.reduce( - (sum, action) => sum + (action.kind === 'wait' ? action.timeoutMs + 5_000 : 35_000), - 0, - ), + Math.min( + WORKFLOW_RUN_TIMEOUT_CEILING_MS, + workflow.actions.reduce( + (sum, action) => sum + (action.kind === 'wait' ? action.timeoutMs + 5_000 : 35_000), + 0, + ), + ), );
649-656: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the global recorder registration explicit or reversible.
setBrowserWorkflowNavigationRecorderwrites module-global hooks, and the factory calls it unconditionally. A secondcreateBrowserWorkflowServicecall replaces the hooks of the first instance. The first service then stops receiving navigation and recorder events, and it fails silently.Production creates one service, so this is not an active defect. Return a dispose handle from the service, or state the single-instance constraint in a comment, so a future second instance does not break recording.
apps/desktop/src/renderer/styles/chat-detail.css (1)
922-925: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the positional selector with a class.
> :nth-child(2)binds the sizing to the DOM order of the wait editor. If a control is inserted before the input, the flex sizing applies to the wrong element. A dedicated class on the input keeps the rule stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d837ec56-4ce2-467f-bba1-12ab35fb4828
📒 Files selected for processing (32)
apps/desktop/e2e/browser-workflow.spec.tsapps/desktop/e2e/fixtures.tsapps/desktop/src/main/__tests__/browser-workflow.test.tsapps/desktop/src/main/__tests__/use-browser-workflow-session-lifecycle.test.tsapps/desktop/src/main/browser-ipc-main.tsapps/desktop/src/main/browser/browser-workflow-service.tsapps/desktop/src/main/browser/controller.tsapps/desktop/src/main/browser/workflow-recorder.tsapps/desktop/src/main/browser/workflow-runner.tsapps/desktop/src/main/runtime-host-boot.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/app-shell.tsxapps/desktop/src/renderer/browser-panel.tsxapps/desktop/src/renderer/browser-workflow-page.tsxapps/desktop/src/renderer/locales/browser-copy.tsapps/desktop/src/renderer/locales/browser-workflow-copy.tsapps/desktop/src/renderer/locales/shell-copy.tsapps/desktop/src/renderer/nav-selection.tsapps/desktop/src/renderer/styles/chat-detail.cssapps/desktop/src/renderer/use-browser-workflow-session-lifecycle.tsdocs/astryx-surface-file-inventory.mddocs/astryx-surface-file-inventory.pathspackages/core/package.jsonpackages/core/src/__tests__/browser-workflow.test.tspackages/core/src/browser-workflow.tspackages/storage/src/__tests__/browser-workflow-store.test.tspackages/storage/src/browser-workflow-store.tspackages/storage/src/index.tspackages/ui/src/module-hub-selector.tsxpackages/ui/src/nav-selection.tspackages/ui/src/shared-ui-copy.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
| if (previousSessionId && previousSessionId !== nextSessionId) { | ||
| void browserWorkflows.releaseSession(previousSessionId); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Contain browserWorkflows.releaseSession rejections at every call site. No caller handles a rejection from the workflow session release. In the IPC handlers the promise is discarded with void, so a rejection becomes an unhandled rejection in the main process. In the boot callback the rejection propagates and skips the browser session cleanup that must still run.
apps/desktop/src/main/browser-ipc-main.ts#L94-L96: attach a.catchthat logs, so the discarded promise cannot produce an unhandled rejection.apps/desktop/src/main/browser-ipc-main.ts#L176-L183: attach the same.catchto thevoid browserWorkflows.releaseSession(resourceKey)call in thebrowser:workflow-release-sessionhandler.apps/desktop/src/main/runtime-host-boot.ts#L515-L518: catch and log thebrowserIpc.releaseSessionrejection soreleaseBrowserSession(sessionId)always runs.
📍 Affects 2 files
apps/desktop/src/main/browser-ipc-main.ts#L94-L96(this comment)apps/desktop/src/main/browser-ipc-main.ts#L176-L183apps/desktop/src/main/runtime-host-boot.ts#L515-L518
| wc.on('console-message', (_event, _level, message) => { | ||
| const credential = this.workflowRecorderCredential; | ||
| if (!credential) return; | ||
| const recorderEvent = parseBrowserWorkflowRecorderConsoleMessage(message, credential); | ||
| if (recorderEvent !== null) { | ||
| notifyBrowserWorkflowRecorderEvent(this.sessionId, recorderEvent); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Electron 43 webContents console-message event signature deprecated positional arguments
💡 Result:
In Electron, the positional arguments for the console-message event on WebContents (specifically level, message, line, and sourceId) have been deprecated in favor of a single object parameter [1][2]. The updated event signature now passes an object containing all relevant details, including a new frame property [1][3]. Deprecated Signature: webContents.on('console-message', (event, level, message, line, sourceId) => {... }) Recommended Replacement: webContents.on('console-message', ({ level, message, lineNumber, sourceId, frame }) => {... }) Key Changes: - Arguments are now consolidated into a single object parameter [1][4]. - The level parameter is now a string (e.g., 'info', 'warning', 'error', 'debug') rather than an integer [1][3]. - A new frame (WebFrameMain) property is included in the event object [3][4]. - The deprecated signature will continue to function but triggers a deprecation warning when used [5][6].
Citations:
- 1: https://electronjs.org/docs/latest/breaking-changes
- 2: https://github.com/electron/electron/blob/1ad832a4/docs/breaking-changes.md
- 3: https://electronjs.org/docs/latest/api/web-contents
- 4: feat: expose frame & move properties to console-message event object electron/electron#43617
- 5: https://github.com/electron/electron/blob/main/lib/browser/api/web-contents.ts
- 6: electron/electron@c868963
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Electron version declarations ---'
rg -n --hidden --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' \
'(^|["'\''])electron(["'\'']|[[:space:]]*:)|electron@|ELECTRON' \
package.json apps packages pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | head -200
printf '%s\n' '--- Relevant handler and parser definitions/usages ---'
rg -n -C 6 --hidden --glob '!node_modules/**' \
'console-message|parseBrowserWorkflowRecorderConsoleMessage|workflowRecorderCredential|100.?ms|drain' \
apps/desktop/src apps/desktop 2>/dev/null | head -300Repository: maka-agent/maka-agent
Length of output: 40440
🌐 Web query:
Electron 43 webContents console-message event API signature version 43 release breaking changes
💡 Result:
In Electron 43, the webContents console-message event signature has been updated, moving from individual arguments to a single event object [1][2]. Previous API Signature (Deprecated): webContents.on('console-message', (event, level, message, line, sourceId) => {}) New API Signature: webContents.on('console-message', ({ level, message, lineNumber, sourceId, frame }) => {}) Additionally, in the new signature, the level argument is now a string with the possible values of 'info', 'warning', 'error', and 'debug' [1][2]. This change was part of a planned deprecation of the older argument-based event emitter [1][2]. While the Electron 43 release notes highlight several major breaking changes—such as the default file download directory behavior, nativeImage SRGB normalization, and removal of dialog support—the shift in the console-message event structure reflects a broader ongoing effort to modernize Electron APIs [3][4][5].
Citations:
- 1: https://contextqmd.com/libraries/electron/versions/41.2.0/pages/docs/breaking-changes
- 2: electron/electron@c868963
- 3: https://electronjs.org/blog/electron-43-0
- 4: https://releases.electronjs.org/release/v43.0.0
- 5: https://www.electronjs.org/blog/electron-43-0
🌐 Web query:
site:github.com/electron/electron/tree/v43.2.0 console-message webContents event object
💡 Result:
In Electron v43.2.0, the webContents 'console-message' event provides a details object as its first argument:
webContents.on('console-message', (details) => {
console.log(details.message)
console.log(details.level)
console.log(details.lineNumber)
console.log(details.sourceId)
console.log(details.frame)
})Properties:
message: console message textlevel:info,warning,error, ordebuglineNumber: source line numbersourceId: source URLframe:WebFrameMainthat logged the message
The older positional arguments (level, message, line, sourceId) remain deprecated for compatibility. (electronjs.org)
Citations:
🌐 Web query:
site:github.com/electron/electron/blob/v43.2.0 docs/api/web-contents.md "console-message"
💡 Result:
In Electron v43.2.0, the 'console-message' event is documented at line 949. It provides the preferred details object:
messagelevellineNumbersourceIdframe
The older positional arguments (level, message, line, sourceId) are deprecated. (github.com)
webContents.on('console-message', (details) => {
console.log(details.level, details.message)
})Citations:
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Parser implementation ---'
rg -n -A 35 -B 5 \
'export function parseBrowserWorkflowRecorderConsoleMessage|function parseBrowserWorkflowRecorderConsoleMessage' \
apps/desktop/src/main/browser/workflow-recorder.ts
printf '%s\n' '--- Standalone callback argument probe ---'
node - <<'JS'
const details = { level: 'info', message: '__MAKA_BROWSER_WORKFLOW_EVENT_V1__:credential:{}' };
((_event, _level, message) => {
console.log(JSON.stringify({ message, messageType: typeof message }));
})(details);
JSRepository: maka-agent/maka-agent
Length of output: 2449
🌐 Web query:
Electron v43 console-message "single object" "old signature" compatibility source
💡 Result:
In Electron, the console-message event on WebContents has undergone a breaking change where the signature has transitioned from individual arguments to a single event object [1][2][3]. Old Signature: webContents.on('console-message', (event, level, message, line, sourceId) => {}) [1][4] New Signature: webContents.on('console-message', ({ level, message, lineNumber, sourceId, frame }) => {}) [1][4] Key changes include: 1. Arguments Migration: The individual parameters (level, message, line, sourceId) have been moved into a single details object [2][3]. 2. Enhanced Data: A new frame property (of type WebFrameMain) is now included in the object [2][3][5]. 3. Level Format: The level argument is now represented as a string (with possible values: 'info', 'warning', 'error', 'debug') rather than an integer [1][5]. Compatibility and Deprecation: Electron has implemented a deprecation warning when the console-message event is emitted to listeners that still use the old signature (i.e., listeners with more than one argument) [2][3]. Developers should migrate to the new object-based signature to avoid these warnings and ensure future compatibility [1][4]. The documentation now reflects the new structure as the standard [5].
Citations:
- 1: https://github.com/electron/electron/blob/main/docs%2Fbreaking-changes.md
- 2: feat: expose frame & move properties to console-message event object electron/electron#43617
- 3: feat: expose frame & move properties to console-message event object electron/electron#43617
- 4: https://electronjs.org/docs/latest/breaking-changes
- 5: https://electronjs.org/docs/latest/api/web-contents
Use the object-form console-message handler. Electron 43.2.0 emits one details object and deprecates the positional callback. The current callback leaves message undefined, so parseBrowserWorkflowRecorderConsoleMessage throws at message.startsWith. Read event.message before parsing.
| onRun={async (workflowId, sensitiveValues) => { | ||
| if (!activeId) throw new Error('No active browser session.'); | ||
| setNavSelection({ section: 'sessions', filter: 'chats' }); | ||
| setWorkbarCollapsed(false); | ||
| openWorkbarTab('browser', 'right'); | ||
| const browserReady = waitForBrowserSessionLive(activeId); | ||
| await window.maka.browser.prepare(activeId); | ||
| await browserReady; | ||
| await window.maka.browser.workflows.run(workflowId, activeId, sensitiveValues); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Observe browserReady when browser.prepare() fails.
If browser.prepare(activeId) rejects, line 2939 exits before awaiting browserReady. Its timeout then rejects five seconds later without a handler.
Await both operations together so the readiness rejection stays handled.
Proposed fix
- await window.maka.browser.prepare(activeId);
- await browserReady;
+ await Promise.all([
+ window.maka.browser.prepare(activeId),
+ browserReady,
+ ]);📝 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.
| onRun={async (workflowId, sensitiveValues) => { | |
| if (!activeId) throw new Error('No active browser session.'); | |
| setNavSelection({ section: 'sessions', filter: 'chats' }); | |
| setWorkbarCollapsed(false); | |
| openWorkbarTab('browser', 'right'); | |
| const browserReady = waitForBrowserSessionLive(activeId); | |
| await window.maka.browser.prepare(activeId); | |
| await browserReady; | |
| await window.maka.browser.workflows.run(workflowId, activeId, sensitiveValues); | |
| onRun={async (workflowId, sensitiveValues) => { | |
| if (!activeId) throw new Error('No active browser session.'); | |
| setNavSelection({ section: 'sessions', filter: 'chats' }); | |
| setWorkbarCollapsed(false); | |
| openWorkbarTab('browser', 'right'); | |
| const browserReady = waitForBrowserSessionLive(activeId); | |
| await Promise.all([ | |
| window.maka.browser.prepare(activeId), | |
| browserReady, | |
| ]); | |
| await window.maka.browser.workflows.run(workflowId, activeId, sensitiveValues); |
| 'nav:automations': [ | ||
| 'automations', | ||
| 'plan', | ||
| 'reminder', | ||
| 'schedule', | ||
| 'cron', | ||
| '自动任务', | ||
| '定时任务', | ||
| '计划', | ||
| '提醒', | ||
| '操作流程', | ||
| ], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add an English workflow keyword.
The Browser workflows surface has no English workflow keyword. Command-palette search for “workflow” will not match this navigation entry.
'nav:automations': [
'automations',
+ 'workflow',
+ 'browser workflow',
'plan',📝 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.
| 'nav:automations': [ | |
| 'automations', | |
| 'plan', | |
| 'reminder', | |
| 'schedule', | |
| 'cron', | |
| '自动任务', | |
| '定时任务', | |
| '计划', | |
| '提醒', | |
| '操作流程', | |
| ], | |
| 'nav:automations': [ | |
| 'automations', | |
| 'workflow', | |
| 'browser workflow', | |
| 'plan', | |
| 'reminder', | |
| 'schedule', | |
| 'cron', | |
| '自动任务', | |
| '定时任务', | |
| '计划', | |
| '提醒', | |
| '操作流程', | |
| ], |
| useEffect(() => { | ||
| const lifecycle = ++lifecycleRef.current; | ||
| mountedRef.current = true; | ||
| return () => { | ||
| mountedRef.current = false; | ||
| const releasedSessionId = sessionIdRef.current; | ||
| queueMicrotask(() => { | ||
| if (lifecycleRef.current !== lifecycle) return; | ||
| window.maka.browser.workflows.releaseSession(releasedSessionId); | ||
| }); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release the session that the panel stops owning.
The effect never reruns when sessionId changes. Its cleanup reads sessionIdRef.current, so switching from session-first to session-latest never releases session-first. Its recorder or run resources can remain active without a panel that can manage them.
apps/desktop/src/renderer/use-browser-workflow-session-lifecycle.ts#L9-L20: capture the effect-owned session ID and release it when ownership changes. Suppress only a same-session StrictMode effect replay.apps/desktop/src/main/__tests__/use-browser-workflow-session-lifecycle.test.ts#L35-L45: assert thatsession-firstreleases after the rerender, then assert thatsession-latestreleases on unmount.
📍 Affects 2 files
apps/desktop/src/renderer/use-browser-workflow-session-lifecycle.ts#L9-L20(this comment)apps/desktop/src/main/__tests__/use-browser-workflow-session-lifecycle.test.ts#L35-L45
| | `apps/desktop/src/renderer/artifact-preview.tsx` | shell-chrome-or-panel | Banner, Button, Spinner | aligned — uses Astryx (Banner, Button, Spinner) | aligned | | ||
| | `apps/desktop/src/renderer/browser-panel.tsx` | shell-chrome-or-panel | EmptyState, IconButton, Toolbar, Tooltip | aligned — uses Astryx (EmptyState, IconButton, Toolbar, Tooltip) | aligned | | ||
| | `apps/desktop/src/renderer/browser-panel.tsx` | shell-chrome-or-panel | EmptyState, IconButton, SegmentedControl, SegmentedControlItem, Toolbar, Tooltip | aligned — uses Astryx (EmptyState, IconButton, SegmentedControl, SegmentedControlItem, Toolbar, Tooltip) | aligned | | ||
| | `apps/desktop/src/renderer/browser-workflow-page.tsx` | shell-chrome-or-panel | Button, EmptyState | aligned — uses Astryx (Button, EmptyState) | aligned | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List the Astryx components imported by the new workflow page.
fd -t f 'browser-workflow-page.tsx' apps/desktop/src --exec rg -n -A15 "from '`@astryxdesign`|from \"`@astryxdesign`|from '`@maka/ui`" {}
fd -t f 'browser-workflow-page.tsx' apps/desktop/src --exec rg -n '<[A-Z][A-Za-z]*' {}Repository: maka-agent/maka-agent
Length of output: 2796
🏁 Script executed:
#!/bin/bash
set -e
file=$(fd -t f 'browser-workflow-page.tsx' apps/desktop/src | head -n1)
printf '%s\n' "== imports and JSX =="
sed -n '1,22p;108,190p' "$file"
printf '%s\n' "== inventory row and generator references =="
rg -n -C 3 'browser-workflow-page\.tsx|surface-file-inventory|component list|TextInput|PageHeader' docs scripts apps/desktop/src --glob '!**/node_modules/**' | head -n 240Repository: maka-agent/maka-agent
Length of output: 33755
🏁 Script executed:
#!/bin/bash
set -e
sed -n '35,75p;180,270p;330,430p' scripts/generate-astryx-surface-inventory.mjsRepository: maka-agent/maka-agent
Length of output: 8145
🏁 Script executed:
#!/bin/bash
set -e
sed -n '44,115p;245,335p' scripts/generate-astryx-surface-inventory.mjsRepository: maka-agent/maka-agent
Length of output: 4675
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '== TextInput exports =='
rg -n -C 3 'TextInput' packages/ui/src packages/ui/package.json apps/desktop/src/renderer/browser-workflow-page.tsx
printf '%s\n' '== generator output contract =='
rg -n -C 2 'MAKA_UI_ASTRYX_REEXPORTS|collectAstryxUsage|generated by' scripts/generate-astryx-surface-inventory.mjs docs/astryx-surface-file-inventory.mdRepository: maka-agent/maka-agent
Length of output: 13015
🏁 Script executed:
#!/bin/bash
set -e
sed -n '105,140p' packages/ui/src/index.ts
sed -n '1,18p' apps/desktop/src/renderer/browser-workflow-page.tsxRepository: maka-agent/maka-agent
Length of output: 1223
Fix Astryx detection for TextInput and regenerate the inventory.
browser-workflow-page.tsx uses the TextInput Astryx re-export from @maka/ui, but MAKA_UI_ASTRYX_REEXPORTS omits it. Add TextInput, then regenerate both inventory artifacts.
| automations: { | ||
| title: '定时任务', | ||
| description: '安排定时运行，并回顾本机任务的工作进展。', | ||
| description: '安排定时运行、回顾本机任务进展，并管理可复用的浏览器操作流程。', | ||
| selectorLabel: (module) => `定时任务内容：${module}`, | ||
| scheduledTasks: '定时任务', | ||
| dailyReview: '每日回顾', | ||
| browserWorkflows: '操作流程', | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Finish the hub rename in the title and the selector label.
The automations hub now holds three modules. Line 176 renames modules.automations to '自动任务', and line 266 renames the English value to 'Automations'. The hub title and selectorLabel were not renamed.
Result in English: the hub heading reads "Scheduled tasks", and the tablist aria-label reads "Scheduled task content: Browser workflows". The label contradicts the selected module. The same mismatch exists in Chinese at lines 165 and 167.
Line 170 also shortens the Chinese label to '操作流程', while the description on line 166 and the English label both keep the browser qualifier.
✏️ Proposed copy fix
automations: {
- title: '定时任务',
+ title: '自动任务',
description: '安排定时运行、回顾本机任务进展，并管理可复用的浏览器操作流程。',
- selectorLabel: (module) => `定时任务内容：${module}`,
+ selectorLabel: (module) => `自动任务内容：${module}`,
scheduledTasks: '定时任务',
dailyReview: '每日回顾',
- browserWorkflows: '操作流程',
+ browserWorkflows: '浏览器操作流程',
},Apply the matching change to the English block:
automations: {
- title: 'Scheduled tasks',
+ title: 'Automations',
description: 'Schedule recurring runs, review local task progress, and manage reusable browser workflows.',
- selectorLabel: (module) => `Scheduled task content: ${module}`,
+ selectorLabel: (module) => `Automation content: ${module}`,
scheduledTasks: 'Scheduled tasks',
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
This is a coherent vertical slice despite its size: schema/storage, recorder, replay/cancellation, lifecycle, IPC, and UI all converge on one main-process service. The current head also addresses most of the earlier persistence, preview, draft, action-limit, and session-release concerns.
The branch still needs a current-main rebase: it conflicts in Browser IPC/boot/preload, the latest merge CI is red, and one navigation call uses a removed type field. Two lifecycle/security boundaries also remain. From first principles, session retirement must be bounded even if an embedded renderer is hung, and browser-derived errors must obey the same URL-redaction contract as persisted workflows. The smallest fix is a bounded, idempotent release that never creates a view, plus one shared safe-URL formatter for every progress/error path. Runtime-only sensitive values should also be cleared after each replay instead of remaining in renderer state.
Reviewed with Codex using two independent reviewer agents; I verified the latest head, current-main types/conflicts, service/runner data flow, prior review ledger, and live CI.
中文
虽然体量较大，但这是一个连贯的垂直切片：schema/storage、recorder、replay/cancellation、lifecycle、IPC 和 UI 最终都收敛到一个 main-process service。当前 head 也修复了此前多数持久化、预览、draft、action limit 和 session release 问题。
分支仍需基于当前 main rebase：Browser IPC/boot/preload 有冲突，最新 merge CI 为红，且一个 navigation 调用仍使用已删除的类型字段。另外还有两个生命周期/安全边界。按第一性原理，session retirement 即使遇到 embedded renderer 卡死也必须有界；浏览器派生的错误必须遵守与 workflow 持久化相同的 URL redaction 契约。最小修复是实现有界、幂等且绝不新建 view 的 release，并让所有 progress/error 路径复用一个 safe-URL formatter。运行时敏感值也应在每次 replay 后清除，而不是继续留在 renderer state。
本次由 Codex 配合两个独立 reviewer agent 审查；我核验了最新 head、current-main 类型/冲突、service/runner 数据流、此前 review ledger 和实时 CI。
| activeSessionId={activeId ?? null} | ||
| onRun={async (workflowId, sensitiveValues) => { | ||
| if (!activeId) throw new Error('No active browser session.'); | ||
| setNavSelection({ section: 'sessions', filter: 'chats' }); |
There was a problem hiding this comment.
P1 — Use the current NavSelection contract. This head defines the sessions variant as { section: 'sessions' }; filter no longer exists, so a clean build against the current UI types rejects this excess property. Change this to setNavSelection({ section: 'sessions' }), then rebase and rerun the affected typecheck/E2E.
| async function drain(recording: Recording): Promise<void> { | ||
| if (recording.released) return; | ||
| const view = deps.views.get(recording.sessionId); | ||
| if (!view) return; |
There was a problem hiding this comment.
P2 — Bound recorder drain during session retirement. releaseSession() ultimately awaits drainWorkflowRecorderEvents(), whose isolated-world execution has no timeout. A hung embedded renderer leaves cancel/release pending forever, so session archive/delete cannot finish destroying its BrowserView. Mark the recording released first, apply a bounded drain/stop deadline, and continue cleanup on timeout; add a never-settling drain regression.
| const remainingMs = deadline - Date.now(); | ||
| if (remainingMs <= 0) { | ||
| throw new Error( | ||
| `Browser workflow navigation did not reach ${expectedUrl}; current URL is ${currentUrl || 'unknown'}.`, |
There was a problem hiding this comment.
P2 — Do not put the full runtime URL into cross-process errors. If navigation lands on ...?access_token=secret, this timeout embeds the complete URL in the error, which then crosses IPC through workflow progress/rejection and reaches renderer toast state. Strip userinfo, query, and fragment (or use a fixed placeholder) through one shared safe-URL formatter, and assert the secret is absent from error/progress/UI output.
|
/agentic_review |
Code Review by Qodo
1. Rejected values pass replay
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const result = await page.evaluate<LocatorResult>(resolveLocatorScript(action.locator, 'type', value)); | ||
| assertLocatorResult(result, action.locator); |
There was a problem hiding this comment.
1. Rejected values pass replay 🐞 Bug ≡ Correctness
Fix-now: type actions only validate locator resolution and never compare the control's returned actual value with the requested value, so a changed <select> with no matching option (or another coercing control) silently completes with the wrong value. The workflow then continues and can report success despite not reproducing the recorded state.
Agent Prompt
## Issue description
Type replay ignores the `actual` value returned after applying an input, allowing a rejected or coerced value to count as successful replay.
## Issue Context
The existing locator script already returns the resulting value, so reuse that seam rather than adding a new API or state. Add the smallest local comparison and a deterministic failure; the only added burden should be one mismatch branch and its focused test.
## Fix Focus Areas
- apps/desktop/src/main/browser/workflow-runner.ts[206-217]
- apps/desktop/src/main/__tests__/browser-workflow.test.ts[574-597]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Array.isArray(workflow.actions) && | ||
| workflow.actions.length > 0 && | ||
| workflow.actions.length <= BROWSER_WORKFLOW_MAX_ACTIONS && | ||
| workflow.actions.every(isBrowserWorkflowAction) |
There was a problem hiding this comment.
2. Duplicate action ids accepted 🐞 Bug ≡ Correctness
Fix-now: isBrowserWorkflow validates actions independently but never requires unique action IDs, so a persisted definition can map multiple sensitive steps to the same replay-secret key. Those steps cannot receive distinct values and will all replay with the same secret, while still passing the advertised workflow validation boundary.
Agent Prompt
## Issue description
Workflow validation accepts duplicate action IDs even though action IDs are used as unique keys for replay-time sensitive values.
## Issue Context
Per-action validation cannot establish collection-wide uniqueness, so deletion or reuse of the existing predicate is insufficient. Add one local collection-level uniqueness condition to the existing validator and a focused contract test; this introduces only a validator branch and test case, not new state or public surface.
## Fix Focus Areas
- packages/core/src/browser-workflow.ts[251-268]
- packages/core/src/__tests__/browser-workflow.test.ts[1-216]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
2 participants
Summary
Scope
This PR implements Slice 1: Human record and replay. Agent parity remains Slice 2 and is intentionally not included here.
Verification
git diff --checkpassedPart of #1557.