Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughImplements batch-fitting guardrails for tool outputs: stage tool results, fit a batch against context budget, downgrade tail items to a fixed failure message and clean up offload files, and only commit persisted messages/hooks/search results after fitting; adds tests, i18n keys, splash gating, and MCP store change. Changes
Sequence Diagram(s)sequenceDiagram
participant Executor as executeTools
participant Guard as ToolOutputGuard
participant OffloadFS as OffloadFS
participant MessageStore as MessageStore
participant Conversation as Conversation/Model
Executor->>Guard: prepareToolOutput(toolResult)
Executor->>Executor: collect stagedResults[]
Executor->>Guard: fitToolBatchOutputs(stagedResults, contextBudget)
Guard->>OffloadFS: delete offload files for downgraded items (concurrent) rgba(200,100,50,0.5)
Guard-->>Executor: FitResult (ok | terminal_error) with per-item downgraded flags
alt FitResult.kind == 'ok'
Executor->>Conversation: append tool messages for non-downgraded items
Executor->>MessageStore: addSearchResult(...) for non-downgraded search items
Executor->>Executor: run post-hooks for success items
else FitResult.kind == 'terminal_error'
Executor->>Conversation: append terminal failure message / end turn
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/main/presenter/lifecyclePresenter/SplashWindowManager.display.test.ts (1)
104-120: Tighten this test to main-window-specific suppressionThis case currently proves suppression on a generic
WINDOW_CREATEDemission, which can mask the non-main-window bug. Add/adjust assertions so suppression is tied to a main-window-specific event payload (and ideally keep a negative case for non-main windows).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/lifecyclePresenter/SplashWindowManager.display.test.ts` around lines 104 - 120, The test currently treats any WINDOW_EVENTS.WINDOW_CREATED as "main" and thus can hide non-main bugs; update the emission to use a main-window-specific payload (e.g. call eventBus.sendToMain(WINDOW_EVENTS.WINDOW_CREATED, { id: 1, isMain: true }) or, if your codebase has it, emit WINDOW_EVENTS.MAIN_WINDOW_CREATED) so SplashWindowManager suppression is exercised only for a true main window, and add a short negative case that emits WINDOW_EVENTS.WINDOW_CREATED with isMain: false (or a non-main id) and asserts the splashWindow.show was called and manager.isVisible() is true; reference SplashWindowManager, eventBus.sendToMain, and WINDOW_EVENTS.WINDOW_CREATED (or WINDOW_EVENTS.MAIN_WINDOW_CREATED) when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/lifecyclePresenter/SplashWindowManager.ts`:
- Around line 79-86: The current onMainWindowCreated handler unconditionally
suppresses the splash for any WINDOW_EVENTS.WINDOW_CREATED; update the handler
to only suppress when the created window is the main app window by checking a
main-window-specific signal in the event payload (e.g., payload.isMainWindow or
payload.windowType === 'main') before setting suppressSplashShow and calling
clearSplashShowDelayTimer. Modify the listener registration for
WINDOW_EVENTS.WINDOW_CREATED to pass the event payload into onMainWindowCreated
(or add a new handler e.g., onWindowCreatedForMain) and reference the existing
symbols onMainWindowCreated, WINDOW_EVENTS.WINDOW_CREATED, suppressSplashShow,
and clearSplashShowDelayTimer when making the change. Ensure the same gated
check is applied to the other suppression call at the lines around 104-105 as
well.
---
Nitpick comments:
In `@test/main/presenter/lifecyclePresenter/SplashWindowManager.display.test.ts`:
- Around line 104-120: The test currently treats any
WINDOW_EVENTS.WINDOW_CREATED as "main" and thus can hide non-main bugs; update
the emission to use a main-window-specific payload (e.g. call
eventBus.sendToMain(WINDOW_EVENTS.WINDOW_CREATED, { id: 1, isMain: true }) or,
if your codebase has it, emit WINDOW_EVENTS.MAIN_WINDOW_CREATED) so
SplashWindowManager suppression is exercised only for a true main window, and
add a short negative case that emits WINDOW_EVENTS.WINDOW_CREATED with isMain:
false (or a non-main id) and asserts the splashWindow.show was called and
manager.isVisible() is true; reference SplashWindowManager, eventBus.sendToMain,
and WINDOW_EVENTS.WINDOW_CREATED (or WINDOW_EVENTS.MAIN_WINDOW_CREATED) when
making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03b15e76-1039-4998-be61-4abd4f8f569b
📒 Files selected for processing (22)
docs/specs/tool-output-guardrails/plan.mddocs/specs/tool-output-guardrails/spec.mdsrc/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/deepchatAgentPresenter/toolOutputGuard.tssrc/main/presenter/lifecyclePresenter/SplashWindowManager.tssrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/mcp.tstest/main/presenter/deepchatAgentPresenter/dispatch.test.tstest/main/presenter/deepchatAgentPresenter/process.test.tstest/main/presenter/lifecyclePresenter/SplashWindowManager.display.test.tstest/renderer/stores/mcpStore.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e81886972
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
Localization
Tests