Conversation
- Update HooksNotificationsDeps type to use new agent methods - Replace getConversation with getSession, remove resolveWorkspaceContext - Update buildPayload to extract workdir from session.projectDir directly - Refactor extractPromptPreview to handle JSON serialized content - Adjust presenter initialization order for dependency injection
Contributor
📝 WalkthroughWalkthroughThis PR removes extensive P0 planning/spec docs, updates architecture docs to the new agent presenter layout, bumps package version to 1.0.0-beta.1, adjusts presenter wiring and HooksNotifications API, tweaks YoBrowserPresenter and session creation, injects optimistic messages, and normalizes i18n placeholders. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Presenter
participant HooksNotifications
participant NewAgentPresenter
participant DeepChatAgentPresenter
Note over Presenter: Presenter startup
Presenter->>HooksNotifications: instantiate with placeholder deps\n(getSession: async()=>null, getMessage: async()=>null)
Renderer->>Presenter: UI events / requests
Presenter->>NewAgentPresenter: create NewAgentPresenter
NewAgentPresenter->>DeepChatAgentPresenter: init loop / session APIs
Presenter->>HooksNotifications: re-instantiate with real deps\n(getSession: newAgentPresenter.getSession, getMessage: newAgentPresenter.getMessage)
Renderer->>NewAgentPresenter: createSession(request)
NewAgentPresenter-->>Renderer: return session (early)
NewAgentPresenter->>DeepChatAgentPresenter: process initial message (background)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/index.ts (1)
206-237:⚠️ Potential issue | 🔴 CriticalNewSessionHooksBridge retains reference to stale HooksNotificationsService instance.
The
NewSessionHooksBridgeis created at line 211 with the firsthooksNotificationsinstance (which has placeholder dependencies returningnull). Whenthis.hooksNotificationsis reassigned at lines 234-237 with real dependencies, the bridge still holds a reference to the old instance.Events dispatched through
newSessionHooksBridge(used byDeepChatAgentPresenter) will use the placeholder dependencies that always returnnull, causing session/message lookups to silently fail.🐛 Proposed fix: Create bridge after final service instantiation or use a getter pattern
Option 1: Move bridge creation after final instantiation
// Initialize new agent architecture presenters first (needed by hooksNotifications) - this.hooksNotifications = new HooksNotificationsService(this.configPresenter, { - getSession: async () => null, - getMessage: async () => null - }) - const newSessionHooksBridge = new NewSessionHooksBridge(this.hooksNotifications) // Initialize new agent architecture presenters const deepchatAgentPresenter = new DeepChatAgentPresenter( this.llmproviderPresenter as unknown as ILlmProviderPresenter, this.configPresenter, this.sqlitePresenter as unknown as import('./sqlitePresenter').SQLitePresenter, this.toolPresenter, - newSessionHooksBridge + null // temporarily, will be set after hooksNotifications is ready ) this.newAgentPresenter = new NewAgentPresenter( deepchatAgentPresenter, this.llmproviderPresenter as unknown as ILlmProviderPresenter, this.configPresenter, this.sqlitePresenter as unknown as import('./sqlitePresenter').SQLitePresenter, this.skillPresenter ) // ... projectPresenter init ... // Update hooksNotifications with actual dependencies now that newAgentPresenter is ready this.hooksNotifications = new HooksNotificationsService(this.configPresenter, { getSession: this.newAgentPresenter.getSession.bind(this.newAgentPresenter), getMessage: this.newAgentPresenter.getMessage.bind(this.newAgentPresenter) }) + const newSessionHooksBridge = new NewSessionHooksBridge(this.hooksNotifications) + deepchatAgentPresenter.setHooksBridge(newSessionHooksBridge)Option 2: Use a dispatcher object with mutable reference
+ // Create a dispatcher that delegates to current hooksNotifications + const hooksDispatcher = { + dispatchEvent: (event: HookEventName, context: HookDispatchContext) => { + this.hooksNotifications.dispatchEvent(event, context) + } + } - const newSessionHooksBridge = new NewSessionHooksBridge(this.hooksNotifications) + const newSessionHooksBridge = new NewSessionHooksBridge(hooksDispatcher)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/index.ts` around lines 206 - 237, NewSessionHooksBridge is constructed with an early HooksNotificationsService that has stubbed getters, so move creation or switch to an indirection so the bridge sees the final service: after you finish instantiating this.hooksNotifications with real dependencies (the reassignment that binds this.newAgentPresenter.getSession/getMessage), create the NewSessionHooksBridge and pass it into DeepChatAgentPresenter and NewAgentPresenter (or alternatively make NewSessionHooksBridge accept a mutable provider/getter that looks up this.hooksNotifications at call time), ensuring DeepChatAgentPresenter uses the bridge tied to the final HooksNotificationsService.
🤖 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/browser/YoBrowserPresenter.ts`:
- Around line 585-590: Replace the direct fire-and-forget call to
state.page.contents.loadURL in YoBrowserPresenter (around the block using
state.page.contents.loadURL) with the same guarded navigation flow used by
BrowserTab.ts's page.navigate (or call that method directly): ensure you set
page.status to Loading, call the waitForLoad timeout/handling logic, capture and
set Ready or Error and extract the page title after load, handle and log errors,
and only update state.updatedAt after the load completion/status update so
downstream code won't race on an in-progress load.
In `@src/renderer/src/i18n/da-DK/chat.json`:
- Line 22: The placeholder value currently uses the literal "{'@'}" which will
display those braces instead of the @ character; update the "placeholder" entry
in the chat locale (the "placeholder" key in chat.json) to use a bare "@"
character (i.e., replace "{'@'}" with "@") so the UI renders the mention
character correctly.
In `@src/renderer/src/i18n/en-US/chat.json`:
- Line 3: The placeholder string currently contains the literal sequence "{'@'}"
which renders verbatim; update the "placeholder" value in chat.json to use the
plain @ character (e.g., "Ask DeepChat anything, @ to mention files, / for
commands") so TipTap's Placeholder extension shows @ correctly; locate the
"placeholder" key in the chat.json locale entry and replace the curly-braced
token with a simple @ character (also apply the same change to other locale
files with the same pattern).
In `@src/renderer/src/i18n/fa-IR/chat.json`:
- Line 3: The placeholder translation currently contains the literal
interpolation syntax "{'@'}" which is rendered verbatim by the TipTap
placeholder (see ChatInputBox.vue which uses t('chat.input.placeholder') and CSS
content: attr(data-placeholder)); update the translation value for the key
chat.input.placeholder to use a plain "@" character (replace "{'@'}" with "@")
so the placeholder displays the @ symbol correctly when rendered by the
Placeholder extension.
In `@src/renderer/src/i18n/fr-FR/chat.json`:
- Line 3: The "placeholder" value in chat.json contains the literal sequence
"{'@'}" which renders as text instead of the intended @ character; update the
"placeholder" string used by the i18n key "placeholder" (in the chat.json locale
entry) to use a bare "@" character (replace "{'@'}" with "@") so the UI displays
the mention symbol correctly.
In `@src/renderer/src/i18n/ru-RU/chat.json`:
- Line 3: The placeholder string for the "placeholder" key in chat.json uses the
literal "{'@'}" which renders as those characters instead of the @ symbol;
update the value for "placeholder" in src/renderer/src/i18n/ru-RU/chat.json (the
"placeholder" entry) to use a bare "@" character where needed (remove the braces
and quotes around @) so the UI shows the actual @ symbol.
---
Outside diff comments:
In `@src/main/presenter/index.ts`:
- Around line 206-237: NewSessionHooksBridge is constructed with an early
HooksNotificationsService that has stubbed getters, so move creation or switch
to an indirection so the bridge sees the final service: after you finish
instantiating this.hooksNotifications with real dependencies (the reassignment
that binds this.newAgentPresenter.getSession/getMessage), create the
NewSessionHooksBridge and pass it into DeepChatAgentPresenter and
NewAgentPresenter (or alternatively make NewSessionHooksBridge accept a mutable
provider/getter that looks up this.hooksNotifications at call time), ensuring
DeepChatAgentPresenter uses the bridge tied to the final
HooksNotificationsService.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 060aa211-fe94-425e-930d-94508ea33e6f
📒 Files selected for processing (48)
CHANGELOG.mdGAP_ANALYSIS_SUMMARY.mdP0_DESIGN_DECISIONS.mdP0_IMPLEMENTATION_SUMMARY.mddocs/ARCHITECTURE.mddocs/FLOWS.mddocs/architecture/agent-system.mddocs/architecture/new-ui-implementation-plan.mddocs/specs/p0-implementation/README.mddocs/specs/p0-implementation/feature-01-generating-session-ids/plan.mddocs/specs/p0-implementation/feature-01-generating-session-ids/spec.mddocs/specs/p0-implementation/feature-01-generating-session-ids/tasks.mddocs/specs/p0-implementation/feature-02-input-disable-stop/plan.mddocs/specs/p0-implementation/feature-02-input-disable-stop/spec.mddocs/specs/p0-implementation/feature-02-input-disable-stop/tasks.mddocs/specs/p0-implementation/feature-03-cancel-generating/plan.mddocs/specs/p0-implementation/feature-03-cancel-generating/spec.mddocs/specs/p0-implementation/feature-03-cancel-generating/tasks.mddocs/specs/p0-implementation/feature-04-permission-approval/plan.mddocs/specs/p0-implementation/feature-04-permission-approval/spec.mddocs/specs/p0-implementation/feature-04-permission-approval/tasks.mddocs/specs/p0-implementation/feature-05-session-list-refresh/plan.mddocs/specs/p0-implementation/feature-05-session-list-refresh/spec.mddocs/specs/p0-implementation/feature-05-session-list-refresh/tasks.mddocs/specs/p0-implementation/feature-06-optimistic-messages/plan.mddocs/specs/p0-implementation/feature-06-optimistic-messages/spec.mddocs/specs/p0-implementation/feature-06-optimistic-messages/tasks.mddocs/specs/p0-implementation/feature-07-cache-versioning/plan.mddocs/specs/p0-implementation/feature-07-cache-versioning/spec.mddocs/specs/p0-implementation/feature-07-cache-versioning/tasks.mdpackage.jsonsrc/main/presenter/browser/YoBrowserPresenter.tssrc/main/presenter/hooksNotifications/index.tssrc/main/presenter/index.tssrc/main/presenter/newAgentPresenter/index.tssrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-TW/chat.jsonsrc/renderer/src/stores/ui/session.ts
💤 Files with no reviewable changes (25)
- docs/specs/p0-implementation/feature-04-permission-approval/tasks.md
- docs/specs/p0-implementation/feature-02-input-disable-stop/plan.md
- docs/specs/p0-implementation/feature-06-optimistic-messages/spec.md
- docs/specs/p0-implementation/feature-05-session-list-refresh/spec.md
- docs/specs/p0-implementation/feature-06-optimistic-messages/tasks.md
- docs/specs/p0-implementation/feature-01-generating-session-ids/spec.md
- docs/specs/p0-implementation/feature-06-optimistic-messages/plan.md
- docs/specs/p0-implementation/feature-03-cancel-generating/tasks.md
- docs/specs/p0-implementation/feature-05-session-list-refresh/tasks.md
- P0_IMPLEMENTATION_SUMMARY.md
- docs/specs/p0-implementation/feature-01-generating-session-ids/plan.md
- docs/specs/p0-implementation/feature-04-permission-approval/spec.md
- docs/specs/p0-implementation/feature-02-input-disable-stop/tasks.md
- docs/specs/p0-implementation/feature-07-cache-versioning/spec.md
- docs/specs/p0-implementation/feature-04-permission-approval/plan.md
- docs/specs/p0-implementation/feature-07-cache-versioning/tasks.md
- docs/specs/p0-implementation/feature-05-session-list-refresh/plan.md
- docs/specs/p0-implementation/README.md
- docs/specs/p0-implementation/feature-02-input-disable-stop/spec.md
- docs/specs/p0-implementation/feature-03-cancel-generating/spec.md
- GAP_ANALYSIS_SUMMARY.md
- docs/specs/p0-implementation/feature-07-cache-versioning/plan.md
- docs/specs/p0-implementation/feature-03-cancel-generating/plan.md
- docs/specs/p0-implementation/feature-01-generating-session-ids/tasks.md
- P0_DESIGN_DECISIONS.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/main/presenter/YoBrowserPresenter.test.ts (1)
21-27: Consider adding a clarifying comment for the auto-emit behavior.The implementation correctly simulates the async load completion by using
queueMicrotask, which aligns with the production code's Promise-baseddid-finish-loadlistener registration pattern. However, this "magic" auto-emit behavior could be surprising to future maintainers.📝 Suggested documentation
once = vi.fn((event: string, listener: (...args: any[]) => void) => { super.once(event, listener) + // Auto-emit did-finish-load to prevent tests from hanging when awaiting + // presenter methods that wait for page load completion if (event === 'did-finish-load') { queueMicrotask(() => this.emit('did-finish-load')) } return this })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/YoBrowserPresenter.test.ts` around lines 21 - 27, Add a short clarifying comment inside the overridden once implementation (the mocked once function that checks for event === 'did-finish-load' and uses queueMicrotask to emit) explaining that the queueMicrotask auto-emit simulates the asynchronous load-complete behavior from production code (where did-finish-load is listened for via a Promise), so future maintainers understand this “magic” emit is intentional and required for the test timing semantics. Mention the relation to the production listener pattern and why queueMicrotask is used instead of an immediate emit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/main/presenter/YoBrowserPresenter.test.ts`:
- Around line 21-27: Add a short clarifying comment inside the overridden once
implementation (the mocked once function that checks for event ===
'did-finish-load' and uses queueMicrotask to emit) explaining that the
queueMicrotask auto-emit simulates the asynchronous load-complete behavior from
production code (where did-finish-load is listened for via a Promise), so future
maintainers understand this “magic” emit is intentional and required for the
test timing semantics. Mention the relation to the production listener pattern
and why queueMicrotask is used instead of an immediate emit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cede3c9-c4c1-4aed-85c2-9216b0f6a7d8
📒 Files selected for processing (2)
src/main/presenter/browser/YoBrowserPresenter.tstest/main/presenter/YoBrowserPresenter.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/presenter/browser/YoBrowserPresenter.ts
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
Chores
Documentation