refactor(document-generation-wizard): focuses on Inline Wizard hook and document generation takeover view#1106
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughRemoves the 625-line ChangesInlineWizard Hook Decomposition and DocumentGenerationView Modularization
Sequence Diagram(s)sequenceDiagram
participant UI
participant useInlineWizard
participant conversationActions
participant generationActions
participant tabState as useInlineWizardTabState
participant maestro as window.maestro
UI->>useInlineWizard: startWizard(params)
useInlineWizard->>conversationActions: startWizard()
conversationActions->>maestro: autorun.listDocs() [iterate mode]
maestro-->>conversationActions: existingDocs
conversationActions->>tabState: setTabState({isActive, mode, conversation})
UI->>useInlineWizard: sendMessage(content)
useInlineWizard->>conversationActions: sendMessage()
conversationActions->>tabState: setTabState({isWaiting, conversation+userMsg})
conversationActions->>maestro: sendWizardMessage()
maestro-->>conversationActions: {confidence, ready, content}
conversationActions->>tabState: setTabState({confidence, ready, conversation+assistantMsg})
UI->>useInlineWizard: generateDocuments()
useInlineWizard->>generationActions: generateDocuments()
generationActions->>maestro: generateInlineDocuments()
maestro-->>generationActions: onProgress / onDocumentComplete / onComplete
generationActions->>tabState: setTabState({isGeneratingDocs, generatedDocuments, subfolderName})
UI->>useInlineWizard: endWizard(tabId)
useInlineWizard->>conversationActions: endWizard()
conversationActions->>tabState: setTabStates (remove tab)
conversationActions->>maestro: endInlineWizardConversation()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 splits the 1,500-line
Confidence Score: 4/5This is a large structural refactoring with no logic changes; the extracted modules are faithful ports of the original monolith and are covered by new unit tests. The only actionable finding is the deprecated The split is clean, public barrel exports are preserved, service import paths are updated correctly, and the full test suite (31,403 tests) continues to pass. The src/renderer/hooks/batch/inlineWizard/state.ts — the only file with a style issue worth addressing before merge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[useInlineWizard] --> B[useInlineWizardTabState]
A --> C[useInlineWizardSimpleActions]
A --> D[useInlineWizardLifecycleActions]
A --> E[useInlineWizardConversationActions]
A --> F[useInlineWizardGenerationActions]
B --> G[(tabStates Map)]
B --> H[(tabStatesRef)]
B --> I[(conversationSessionsMap ref)]
B --> J[(previousUIStateRefsMap ref)]
E --> K[conversationActions.ts]
K --> L[inlineWizardConversation service]
K --> M[documents.ts helpers]
F --> N[generationActions.ts]
N --> O[inlineWizardDocumentGeneration service]
D --> P[lifecycleActions.ts]
P --> L
subgraph DocumentGenerationView
Q[DocumentGenerationView shell] --> R[GenerationStatus]
Q --> S[CreatedFilesList]
S --> T[CreatedFileEntry]
Q --> U[GenerationActions]
Q --> V[EmptyGenerationState]
Q --> W[useElapsedGenerationTime hook]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[useInlineWizard] --> B[useInlineWizardTabState]
A --> C[useInlineWizardSimpleActions]
A --> D[useInlineWizardLifecycleActions]
A --> E[useInlineWizardConversationActions]
A --> F[useInlineWizardGenerationActions]
B --> G[(tabStates Map)]
B --> H[(tabStatesRef)]
B --> I[(conversationSessionsMap ref)]
B --> J[(previousUIStateRefsMap ref)]
E --> K[conversationActions.ts]
K --> L[inlineWizardConversation service]
K --> M[documents.ts helpers]
F --> N[generationActions.ts]
N --> O[inlineWizardDocumentGeneration service]
D --> P[lifecycleActions.ts]
P --> L
subgraph DocumentGenerationView
Q[DocumentGenerationView shell] --> R[GenerationStatus]
Q --> S[CreatedFilesList]
S --> T[CreatedFileEntry]
Q --> U[GenerationActions]
Q --> V[EmptyGenerationState]
Q --> W[useElapsedGenerationTime hook]
end
Reviews (1): Last reviewed commit: "Refactor inline wizard internals" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+3
to
+8
| /** | ||
| * Generate a unique message ID. | ||
| */ | ||
| export function generateMessageId(): string { | ||
| return `iwm-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; | ||
| } |
There was a problem hiding this comment.
generateMessageId function re-implements ID generation with the deprecated .substr() method. CLAUDE.md designates generateId() from src/renderer/utils/ids.ts as the canonical ID generator to avoid duplicated implementations. This refactoring was a good opportunity to consolidate to the shared utility.
Suggested change
| /** | |
| * Generate a unique message ID. | |
| */ | |
| export function generateMessageId(): string { | |
| return `iwm-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; | |
| } | |
| import { generateId } from '../../../utils/ids'; | |
| /** | |
| * Generate a unique message ID. | |
| */ | |
| export function generateMessageId(): string { | |
| return `iwm-${generateId()}`; | |
| } |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/__tests__/renderer/components/InlineWizard/DocumentGenerationView/useElapsedGenerationTime.test.tsx (1)
25-37: ⚡ Quick winAdd a true→false transition test for elapsed finalization.
Current cases miss the stop-transition path (
isGeneratingbecomes false). A rerender-based test would lock in the final elapsed value and guard against stale-timer regressions.🤖 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/__tests__/renderer/components/InlineWizard/DocumentGenerationView/useElapsedGenerationTime.test.tsx` around lines 25 - 37, Add a new test case after the existing test in this file that verifies the true-to-false transition behavior for the isGenerating prop. The test should render the useElapsedGenerationTime hook with isGenerating as true and a start time, advance timers by some amount to let the elapsed time accumulate, then use rerender to change isGenerating to false, advance timers again, and assert that the elapsed time remains constant at the value it had when the transition occurred (verifying the timer was properly stopped and the value finalized). This guards against stale timer regressions when generation completes.
🤖 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/renderer/components/InlineWizard/DocumentGenerationView/components/EmptyGenerationState.tsx`:
- Around line 16-18: The button element with the onCancel onClick handler is
missing an explicit type attribute, which could cause unintended form submission
if the button is rendered inside a form container. Add type="button" to the
button element in the EmptyGenerationState component to ensure it only triggers
the onCancel callback without submitting any parent form.
In
`@src/renderer/components/InlineWizard/DocumentGenerationView/components/GenerationActions.tsx`:
- Around line 18-33: Both button elements in GenerationActions are missing
explicit type attributes, which can cause unintended form submissions in form
contexts. Add type="button" attribute to the button element with
onClick={onComplete} (the "Exit Wizard" button) and to the button element with
onClick={onCompleteAndStartAutoRun} to ensure they function as regular buttons
and do not trigger form submission behavior.
In
`@src/renderer/components/InlineWizard/DocumentGenerationView/DocumentGenerationView.tsx`:
- Around line 64-67: The Cancel button in DocumentGenerationView is missing an
explicit type attribute, which can cause it to behave as a form submit button if
wrapped in a form element. Locate the button element with onClick={onCancel} and
className containing "mt-4 px-4 py-2", and add type="button" as an explicit
attribute to ensure it only triggers the cancel action without submitting any
parent form.
In
`@src/renderer/components/InlineWizard/DocumentGenerationView/hooks/useElapsedGenerationTime.ts`:
- Around line 8-18: The useElapsedGenerationTime hook in the useEffect
dependency array has isGenerating and startTime, but when isGenerating becomes
false, the cleanup function only clears the interval without performing a final
update to setElapsedMs. This causes the displayed elapsed time to potentially be
stale by up to one second. Add a final setElapsedMs call that executes when
isGenerating transitions to false to capture the most recent elapsed time before
the interval stops updating.
In `@src/renderer/hooks/batch/inlineWizard/conversationActions.ts`:
- Around line 307-350: The sendWizardMessage function already triggers
callbacks.onError internally when there's an error, causing duplicate error
callback emissions. Remove the redundant callbacks?.onError?.(errorMessage) call
in the else block that handles errors from sendWizardMessage, keeping only the
logger.error call and setTabState update in that error handling section.
In `@src/renderer/hooks/batch/inlineWizard/documents.ts`:
- Around line 56-57: The condition in the documents.ts file that checks
`result.success && result.content` incorrectly treats empty file contents as
failures because empty strings are falsy in JavaScript. Change the condition to
only check `result.success`, or alternatively use a more explicit check that
distinguishes between undefined/null and empty strings (such as checking
`result.content !== null && result.content !== undefined` or `result.content !=
null`). This ensures that successfully read empty documents are still added to
the docsWithContent array.
In `@src/renderer/hooks/batch/inlineWizard/lifecycleActions.ts`:
- Around line 46-48: In the useInlineWizard hook in the lifecycleActions.ts
file, the catch blocks at lines 46-48 and 62-64 are currently only logging
cleanup failures without reporting them to Sentry. Import captureException from
Sentry at the top of the file (if not already imported), then in both catch
blocks (the one after the failed conversation session end and the one at line
62-64), add a call to captureException(error) alongside or instead of the
logger.warn call to ensure these cleanup failures are tracked in production
telemetry. Optionally, you can also add a captureMessage call before or after
captureException to provide additional context about which cleanup operation
failed.
In `@src/renderer/hooks/batch/inlineWizard/state.ts`:
- Around line 6-8: The generateMessageId() function in the state.ts file is
reimplementing ID generation locally instead of using shared utilities. Replace
this function's implementation to use either generateId() from
src/renderer/utils/ids.ts or generateUUID() from src/shared/uuid.ts to maintain
consistent ID generation behavior across the codebase, then remove the local
reimplementation.
---
Nitpick comments:
In
`@src/__tests__/renderer/components/InlineWizard/DocumentGenerationView/useElapsedGenerationTime.test.tsx`:
- Around line 25-37: Add a new test case after the existing test in this file
that verifies the true-to-false transition behavior for the isGenerating prop.
The test should render the useElapsedGenerationTime hook with isGenerating as
true and a start time, advance timers by some amount to let the elapsed time
accumulate, then use rerender to change isGenerating to false, advance timers
again, and assert that the elapsed time remains constant at the value it had
when the transition occurred (verifying the timer was properly stopped and the
value finalized). This guards against stale timer regressions when generation
completes.
🪄 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: 4bb44a82-34e5-4911-90e4-4ec7d2e2f107
📒 Files selected for processing (35)
src/__tests__/renderer/components/InlineWizard/DocumentGenerationView/CreatedFilesList.test.tsxsrc/__tests__/renderer/components/InlineWizard/DocumentGenerationView/DocumentGenerationView.test.tsxsrc/__tests__/renderer/components/InlineWizard/DocumentGenerationView/useElapsedGenerationTime.test.tsxsrc/__tests__/renderer/components/InlineWizard/DocumentGenerationView/utils.test.tssrc/__tests__/renderer/components/InlineWizard/DocumentGenerationView/wrappers.test.tsxsrc/__tests__/renderer/hooks/batch/inlineWizard/documents.test.tssrc/__tests__/renderer/hooks/batch/inlineWizard/generationActions.test.tssrc/__tests__/renderer/hooks/batch/inlineWizard/lifecycleActions.test.tsxsrc/__tests__/renderer/hooks/batch/inlineWizard/simpleActions.test.tssrc/__tests__/renderer/hooks/batch/inlineWizard/useInlineWizardTabState.test.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView/DocumentGenerationView.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView/components/CreatedFileEntry.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView/components/CreatedFilesList.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView/components/DocumentEditor.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView/components/DocumentSelector.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView/components/EmptyGenerationState.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView/components/GenerationActions.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView/components/GenerationStatus.tsxsrc/renderer/components/InlineWizard/DocumentGenerationView/components/index.tssrc/renderer/components/InlineWizard/DocumentGenerationView/hooks/useElapsedGenerationTime.tssrc/renderer/components/InlineWizard/DocumentGenerationView/index.tssrc/renderer/components/InlineWizard/DocumentGenerationView/types.tssrc/renderer/components/InlineWizard/DocumentGenerationView/utils/documentStats.tssrc/renderer/hooks/batch/inlineWizard/conversationActions.tssrc/renderer/hooks/batch/inlineWizard/documents.tssrc/renderer/hooks/batch/inlineWizard/generationActions.tssrc/renderer/hooks/batch/inlineWizard/lifecycleActions.tssrc/renderer/hooks/batch/inlineWizard/simpleActions.tssrc/renderer/hooks/batch/inlineWizard/state.tssrc/renderer/hooks/batch/inlineWizard/types.tssrc/renderer/hooks/batch/inlineWizard/useInlineWizardTabState.tssrc/renderer/hooks/batch/useInlineWizard.tssrc/renderer/services/inlineWizardConversation.tssrc/renderer/services/inlineWizardDocumentGeneration.ts
💤 Files with no reviewable changes (1)
- src/renderer/components/InlineWizard/DocumentGenerationView.tsx
Collaborator
|
@reachrazamair thank you for this contribution, it's a genuinely high-quality refactor. 🙏 I reviewed the PR alongside the Greptile and CodeRabbit feedback and everything checks out:
The only outstanding item is CodeRabbit's soft "Docstring Coverage" pre-merge warning, which is non-blocking and consistent with the surrounding code, so I'm not asking for changes there. Approving and tagging |
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
Refactors the Inline Wizard hook and document generation takeover view into smaller SOC/SRP-driven modules while preserving existing behavior, public imports, UI copy, tab routing semantics, and generation flow.
Changes
useInlineWizardinto a small public composer plus internal modules for:inlineWizard/types.tsand preserved existing type re-exports.DocumentGenerationView.tsxinto a directory-based shell with extracted:DocumentSelectorandDocumentEditorwrappersBehavior Delta
None intended.
Existing app behavior, UI layout, public props, public exports, tab sync semantics, SSH/custom override flow, conductor profile forwarding, Auto Run folder handling, and generation service behavior are intended to remain unchanged.
Tests
Passed:
npm run test -- src/__tests__/renderer/hooks/useInlineWizard.test.ts src/__tests__/renderer/hooks/useInlineWizard_overrides.test.ts src/__tests__/renderer/contexts/InlineWizardContext.test.tsx src/__tests__/renderer/components/InlineWizard src/__tests__/integration/InlineWizardFlow.test.tsx src/__tests__/renderer/components/Wizard/sharednpm run test -- src/__tests__/renderer/hooks/batch/inlineWizard src/__tests__/renderer/components/InlineWizard/DocumentGenerationViewnpm run test -- src/__tests__/renderer/hooks/useWizardHandlers.test.ts src/__tests__/renderer/components/MainPanel/MainPanelContent.test.tsx src/__tests__/renderer/components/SessionList.test.tsxnpm run format:checknpm run lintnpm run lint:eslintnpx tsc --noEmit --prettynpm run testFull suite result: 1,099 files passed, 1 skipped; 31,403 tests passed, 108 skipped.
Summary by CodeRabbit
Release Notes