refactor: continue trivial wrapper cleanup#335
Open
Nek-12 wants to merge 15 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR inlines many small helper functions across CLI, TUI, and server code: auth predicates, runtime reducers, UI/TUI rendering and role serialization, onboarding normalization, worktree/session validation, command registry changes, prompt-event wiring, native streaming tail/update behavior, and various service-side helpers. Tests updated where they depended on removed helpers or accessors. ChangesHelper function removal and logic inlining
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: fef5820af3
ℹ️ 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".
| m.nativeStreamingController.Configure(m.nativeStreamingController.theme, msg.Width) | ||
| m.nativeStreamingWidth = msg.Width | ||
| m.nativeStreamingTail = cloneNativeStreamProjectionLines(m.nativeStreamingController.Tail()) | ||
| m.nativeStreamingTail = cloneNativeStreamProjectionLines(m.nativeStreamingController.rendered[m.nativeStreamingController.enqueuedStableLineCount:]) |
There was a problem hiding this comment.
When an assistant stream has already promoted stable lines at a narrow width, widening the terminal can make the re-rendered projection shorter than enqueuedStableLineCount. Configure marks invalidatedByResize for exactly this case, but this slice runs before the invalidation branch, so a WindowSizeMsg during streaming can panic with a slice-bounds error instead of replaying the stream. Check invalidatedByResize or clamp the index before slicing.
Useful? React with 👍 / 👎.
| } | ||
| m.nativeStreamingStableFlushSequence = 0 | ||
| m.nativeStreamingTail = m.nativeStreamingController.Tail() | ||
| m.nativeStreamingTail = cloneNativeStreamProjectionLines(m.nativeStreamingController.rendered[m.nativeStreamingController.enqueuedStableLineCount:]) |
There was a problem hiding this comment.
If a stable streaming flush is awaiting ack and the terminal is resized, the controller may be invalidated and re-rendered to fewer lines than the old enqueuedStableLineCount. This ack path now slices before checking invalidatedByResize, so the next flush ack can still panic even if the resize handler is fixed; use the invalidated full-tail path or clamp before taking the suffix.
Useful? React with 👍 / 👎.
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/app/ui_native_stream_controller.go (1)
64-79:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winClamp stable-line index after resize re-render to prevent slice panics.
Configurecan shrinkc.renderedon width/theme changes while leavingc.enqueuedStableLineCountlarger than the new rendered length. The next stable/tail slice can panic at runtime.Proposed fix
func (c *nativeAssistantStreamController) Configure(theme string, width int) { width = normalizedNativeStreamWidth(width) if theme == "" { theme = "dark" } if width == c.width && theme == c.theme { return } hadStable := c.enqueuedStableLineCount > 0 c.width = width c.theme = theme if c.source != "" { c.invalidatedByResize = c.invalidatedByResize || hadStable c.rendered = tui.RenderAssistantMarkdownProjection(c.source, c.theme, c.width) + if c.enqueuedStableLineCount > len(c.rendered) { + c.enqueuedStableLineCount = len(c.rendered) + } } }🤖 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 `@cli/app/ui_native_stream_controller.go` around lines 64 - 79, Configure can shrink c.rendered when theme/width changes while c.enqueuedStableLineCount may still be larger than the new rendered length, causing later slices to panic; after you set c.rendered in nativeAssistantStreamController.Configure (the block where you call tui.RenderAssistantMarkdownProjection and assign c.rendered), clamp c.enqueuedStableLineCount to at most len(c.rendered) (e.g., set c.enqueuedStableLineCount = min(c.enqueuedStableLineCount, len(c.rendered))) and adjust any related invalidation flags if needed so subsequent stable/tail slicing cannot index past the slice.cli/app/ui_native_stream_controller_test.go (1)
120-135: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a resize reflow regression case where rendered lines decrease.
This test validates invalidation, but it does not exercise the boundary where
Configurereduces rendered line count below previously promoted stable lines. Please add that case and assert no panic through subsequent update/ack flow.As per coding guidelines, "
**/*_test.go: All business logic covered by tests. Production code is written to be unit-testable."🤖 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 `@cli/app/ui_native_stream_controller_test.go` around lines 120 - 135, Add a regression test that exercises the boundary where Configure reduces the rendered line count below already-promoted stable lines: create a controller with newNativeAssistantStreamController(..., 72), Append enough lines to cause promotion to update.stable, then call Configure(controller.theme, 24) to shrink rendered lines, then Append another line and call the subsequent ack/update flow (use controller.Append and controller.Ack or the existing ack path) and assert no panic occurs and the update indicates promotion was invalidated (update.stable is empty), the live tail (update.tail) contains the new content, and update.needsReplay is true; reference the TestNativeAssistantStreamControllerResizeInvalidatesFurtherPromotion test, Configure, Append, Ack, update.stable, update.tail, and update.needsReplay when adding the case.Source: Coding guidelines
🤖 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 `@cli/app/internal/runtimestate/runtime_events_test.go`:
- Around line 34-35: The test currently only asserts that
update.PendingInput.State.Submission is not InputSubmissionLocked; instead
assert it equals the explicit unlocked value to prevent regressions to
zero-value. Update the assertion around update.PendingInput.State.Submission in
the runtime_events_test (the block using InputSubmissionLocked) to compare
against InputSubmissionUnlocked (the expected constant) so the test enforces the
stronger post-condition.
In `@cli/tui/model_rendering.go`:
- Line 134: The detail-rendering closures for reasoning/streaming are discarding
the provided symbolOverride by passing a hardcoded "" into
flattenEntryWithMetaAndSymbol; update each closure that currently calls
model.flattenEntryWithMetaAndSymbol(RenderIntentReasoning, thinkingText, false,
nil, "") (and the similar calls at the other noted sites) to forward the local
symbolOverride variable instead of the empty string so non-default symbol
rendering is preserved.
---
Outside diff comments:
In `@cli/app/ui_native_stream_controller_test.go`:
- Around line 120-135: Add a regression test that exercises the boundary where
Configure reduces the rendered line count below already-promoted stable lines:
create a controller with newNativeAssistantStreamController(..., 72), Append
enough lines to cause promotion to update.stable, then call
Configure(controller.theme, 24) to shrink rendered lines, then Append another
line and call the subsequent ack/update flow (use controller.Append and
controller.Ack or the existing ack path) and assert no panic occurs and the
update indicates promotion was invalidated (update.stable is empty), the live
tail (update.tail) contains the new content, and update.needsReplay is true;
reference the
TestNativeAssistantStreamControllerResizeInvalidatesFurtherPromotion test,
Configure, Append, Ack, update.stable, update.tail, and update.needsReplay when
adding the case.
In `@cli/app/ui_native_stream_controller.go`:
- Around line 64-79: Configure can shrink c.rendered when theme/width changes
while c.enqueuedStableLineCount may still be larger than the new rendered
length, causing later slices to panic; after you set c.rendered in
nativeAssistantStreamController.Configure (the block where you call
tui.RenderAssistantMarkdownProjection and assign c.rendered), clamp
c.enqueuedStableLineCount to at most len(c.rendered) (e.g., set
c.enqueuedStableLineCount = min(c.enqueuedStableLineCount, len(c.rendered))) and
adjust any related invalidation flags if needed so subsequent stable/tail
slicing cannot index past the slice.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f40cf61-9ffc-45a6-be9c-30ed4d9f60c6
📒 Files selected for processing (75)
cli/actions/registry.gocli/app/auth_gate.gocli/app/auth_picker.gocli/app/internal/authinteraction/policy.gocli/app/internal/authinteraction/policy_test.gocli/app/internal/onboardingimportgenerated/generated.gocli/app/internal/onboardingimportskills/skills.gocli/app/internal/onboardingimportskills/skills_test.gocli/app/internal/projectpicker/picker.gocli/app/internal/projectpicker/picker_test.gocli/app/internal/runtimestate/runtime_events.gocli/app/internal/runtimestate/runtime_events_test.gocli/app/internal/sessiontarget/target.gocli/app/internal/sessiontarget/target_test.gocli/app/internal/status/status.gocli/app/internal/submissionerror/errors.gocli/app/internal/submissionerror/errors_test.gocli/app/internal/worktreecreate/create.gocli/app/internal/worktreecreate/create_test.gocli/app/internal/worktreecreateform/form.gocli/app/internal/worktreecreateform/form_test.gocli/app/internal/worktreecreateresolve/resolve.gocli/app/internal/worktreedelete/delete.gocli/app/internal/worktreeselection/selection.gocli/app/onboarding_imports.gocli/app/onboarding_test.gocli/app/project_binding_flow.gocli/app/project_name_prompt.gocli/app/session_lifecycle_test.gocli/app/session_server_target.gocli/app/ui.gocli/app/ui_focus_test.gocli/app/ui_input_resume.gocli/app/ui_input_submission.gocli/app/ui_layout.gocli/app/ui_lifecycle_state.gocli/app/ui_native_history.gocli/app/ui_native_history_projection.gocli/app/ui_native_stream_controller.gocli/app/ui_native_stream_controller_test.gocli/app/ui_part2_test.gocli/app/ui_part6_test.gocli/app/ui_part7_test.gocli/app/ui_part8_test.gocli/app/ui_pending_tools.gocli/app/ui_runtime_adapter.gocli/app/ui_runtime_event_reduction.gocli/app/ui_runtime_status.gocli/app/ui_slash_command_picker_test.gocli/app/ui_status.gocli/app/ui_transcript_entries.gocli/app/ui_transcript_mode.gocli/app/ui_transcript_pager.gocli/app/ui_window_reducer.gocli/app/ui_worktree_commands_test.gocli/app/ui_worktree_create_controller.gocli/app/ui_worktree_list_controller.gocli/app/ui_worktree_render.gocli/builder/service_backend_darwin.gocli/builder/service_backend_windows.gocli/builder/service_types.gocli/tui/ansi_style_transform.gocli/tui/detail_rendering.gocli/tui/model_reducer.gocli/tui/model_rendering.gocli/tui/model_rendering_entries.gocli/tui/model_rendering_entries_test.gocli/tui/model_rendering_style.gocli/tui/model_rendering_tools.gocli/tui/pending_snapshot.gocli/tui/roles.gocli/tui/transcript_intents.gocli/tui/transcript_intents_test.gocli/tui/transcript_projection.goserver/runtimecontrol/service.go
💤 Files with no reviewable changes (12)
- cli/app/internal/submissionerror/errors_test.go
- cli/app/internal/worktreeselection/selection.go
- cli/app/internal/status/status.go
- cli/tui/ansi_style_transform.go
- cli/app/internal/authinteraction/policy.go
- cli/app/ui.go
- cli/app/internal/onboardingimportskills/skills_test.go
- cli/app/internal/sessiontarget/target.go
- cli/tui/transcript_intents.go
- cli/app/internal/sessiontarget/target_test.go
- cli/app/internal/projectpicker/picker_test.go
- cli/app/internal/worktreecreateform/form_test.go
Comment on lines
+34
to
35
| if update.PendingInput.State.Submission == InputSubmissionLocked { | ||
| t.Fatal("expected input submit lock cleared") |
Contributor
There was a problem hiding this comment.
Assert the explicit unlocked state here.
Line 34 only proves the reducer didn't leave Submission as InputSubmissionLocked. After this refactor, the contract is stronger: it should become InputSubmissionUnlocked. As written, a regression to the zero value ("") would still pass.
As per coding guidelines, "All business logic covered by tests. Production code is written to be unit-testable."
Suggested test tightening
- if update.PendingInput.State.Submission == InputSubmissionLocked {
- t.Fatal("expected input submit lock cleared")
+ if update.PendingInput.State.Submission != InputSubmissionUnlocked {
+ t.Fatalf("expected input submit lock cleared to %q, got %q", InputSubmissionUnlocked, update.PendingInput.State.Submission)
}🤖 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 `@cli/app/internal/runtimestate/runtime_events_test.go` around lines 34 - 35,
The test currently only asserts that update.PendingInput.State.Submission is not
InputSubmissionLocked; instead assert it equals the explicit unlocked value to
prevent regressions to zero-value. Update the assertion around
update.PendingInput.State.Submission in the runtime_events_test (the block using
InputSubmissionLocked) to compare against InputSubmissionUnlocked (the expected
constant) so the test enforces the stronger post-condition.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/app/onboarding_render.go (1)
300-304: ⚡ Quick winConsider extracting duplicated normalization logic.
The theme summary formatting (lines 300-304), verbosity normalization (lines 319-322), and reviewer frequency normalization (lines 333-336) duplicate logic from
onboarding_flow.golines 167-170, 188-191, and 196-199 respectively. Extracting these to small helper functions would provide a single source of truth and simplify future updates.♻️ Example extraction for theme summary
// In onboarding_flow.go or a shared location func formatThemeSummary(settingsTheme string) string { themeSummary := theme.Auto + " (" + theme.Resolve(settingsTheme) + ")" if theme.IsExplicit(settingsTheme) { themeSummary = theme.Resolve(settingsTheme) } return themeSummary }Then use
formatThemeSummary(state.settings.Theme)in both files.Also applies to: 319-322, 333-336
🤖 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 `@cli/app/onboarding_render.go` around lines 300 - 304, Extract the duplicated normalization/formatting logic into shared helper functions and call them from both onboarding_render.go and onboarding_flow.go: create formatThemeSummary(settingsTheme string) that encapsulates the sharedtheme.Auto + " (" + sharedtheme.Resolve(...) + ")" and the sharedtheme.IsExplicit(...) branch (used where themeSummary is built for appendRow), create formatVerbositySummary(settingsVerbosity string) to centralize the verbosity normalization used around the verbosity appendRow, and create formatReviewerFrequencySummary(settingsFreq string) for the reviewer frequency normalization; replace the inline logic in onboarding_render.go (where themeSummary, verbosity, and reviewer frequency are computed) with calls to these helpers so both files use a single source of truth.
🤖 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.
Nitpick comments:
In `@cli/app/onboarding_render.go`:
- Around line 300-304: Extract the duplicated normalization/formatting logic
into shared helper functions and call them from both onboarding_render.go and
onboarding_flow.go: create formatThemeSummary(settingsTheme string) that
encapsulates the sharedtheme.Auto + " (" + sharedtheme.Resolve(...) + ")" and
the sharedtheme.IsExplicit(...) branch (used where themeSummary is built for
appendRow), create formatVerbositySummary(settingsVerbosity string) to
centralize the verbosity normalization used around the verbosity appendRow, and
create formatReviewerFrequencySummary(settingsFreq string) for the reviewer
frequency normalization; replace the inline logic in onboarding_render.go (where
themeSummary, verbosity, and reviewer frequency are computed) with calls to
these helpers so both files use a single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bca94b53-13a3-439b-9e0a-6976ac40b68d
📒 Files selected for processing (9)
cli/app/internal/worktreemutation/service.gocli/app/onboarding_flow.gocli/app/onboarding_imports.gocli/app/onboarding_render.gocli/app/onboarding_workflow.gocli/app/ui_layout_rendering_status_overlay.gocli/app/ui_native_scrollback_integration_part5_test.gocli/app/ui_status.gocli/app/ui_worktree_commands.go
💤 Files with no reviewable changes (2)
- cli/app/onboarding_imports.go
- cli/app/ui_status.go
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
Verification
Summary by CodeRabbit
Refactor
Style
Bug Fixes