feat: Supporting streaming components to the catalog#5914
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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:
📝 WalkthroughWalkthroughIntroduces concurrent discovery and bounded parallel loading that streams discovered modules into a redesigned TUI; adds a CatalogService streaming API, exposes keymap/delegate types, updates legacy TUI to use the new APIs, and adds streaming-aware UI logic and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Catalog CLI
participant Discovery as Discovery Goroutines
participant URLCh as Repo URL Channel
participant Loader as Bounded Loader (errgroup/workers)
participant Service as Catalog Service (LoadStreamingURL)
participant TUI as Streaming TUI Model
CLI->>Discovery: start discovery goroutines
par discover catalog-config URLs
Discovery->>URLCh: emit repo URLs
and discover terraform.source URLs
Discovery->>URLCh: emit repo URLs
end
Note over Discovery,URLCh: discovery goroutines close URLCh when done
CLI->>Loader: start bounded workers reading URLCh
loop for each repo URL (deduped)
Loader->>Service: LoadStreamingURL(ctx, l, repoURL, onModule)
Service->>Service: FindModules, call onModule for each module
Service->>Loader: onModule callback invoked
Loader->>TUI: send module over moduleCh
end
Loader->>TUI: send DiscoveryCompleteMsg with CatalogService/Err
TUI->>TUI: insertModuleSorted, update Bubble Tea state
sequenceDiagram
participant User as User
participant Tea as Model.Update
participant List as updateList()
participant Renderer as Glamour Renderer
participant Pager as updatePager()
participant Service as CatalogService
User->>Tea: KeyPressMsg (Enter / shortcuts)
Tea->>Tea: route by m.State
alt ListState
List->>List: match Choose/Scaffold
alt Choose
List->>Renderer: render markdown (if applicable)
Renderer->>List: rendered content
List->>Tea: set PagerState with viewport content
else Scaffold
List->>Service: scaffoldModuleCmd (if m.SVC != nil)
end
else PagerState
Pager->>Pager: handle button/keys
alt Scaffold button/key
Pager->>Service: scaffoldModuleCmd
else View Source
Pager->>Pager: open URL in browser -> show errors in viewport
else Quit
Pager->>Tea: return to ListState
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/commands/catalog/catalog_redesign.go`:
- Around line 75-96: The code currently swallows errors from LoadStreamingURL
(inside loaders.Go) and converts all failures into warnings and ultimately a
nil, nil return when svc.Modules() is empty; change the anonymous function
passed to loaders.Go to return the error from svc.LoadStreamingURL instead of
only logging it so loaders.Wait() can surface failures, and then propagate
errors from loaders.Wait() and g.Wait() by returning them (rather than only
l.Warnf) so callers of this function receive real errors instead of seeing an
empty catalog; ensure references: the anonymous func using
svc.LoadStreamingURL(loadCtx, l, repoURL, onModule), loaders.Wait(), g.Wait(),
and the svc.Modules() empty check are updated accordingly.
- Around line 37-39: The current onModule callback uses a blocking send
(moduleCh <- mod) which can hang the loader goroutine if listenForModule stops
consuming; change onModule to perform an abortable send using the existing
cancellation signal (e.g., a context.Context or quit channel) so the send
becomes select { case moduleCh <- mod: case <-ctx.Done(): return } (or drop the
module on cancel) to ensure the loader goroutine can return; update the caller
that constructs onModule to capture the cancel context/quit channel and use it
in the select so onModule, moduleCh, and listenForModule coordinate shutdown
without blocking.
In `@internal/cli/commands/catalog/tui/model.go`:
- Around line 200-206: The code reads modules from moduleCh and assumes the
received module pointer is non-nil; add explicit nil checks to avoid panics: in
insertModuleSorted ensure the passed-in module (used with mod.Title()) is
checked for nil and return/skip insertion if nil, and in handleModuleMsg verify
msg.module != nil before calling NewModelStreaming (or otherwise handle the nil
case, e.g., log/ignore); also ensure any other handlers that call mod.Title() or
access module fields guard against a nil *module.Module.
In `@internal/cli/commands/catalog/tui/view_test.go`:
- Around line 287-302: The test currently gates all list assertions behind "if
isList" so a failure to transition from WelcomeModel can silently pass; replace
the conditional check with a mandatory assertion that the runtime model is a
list (e.g., assert.True(t, isList, "expected model to transition to tui.Model
list after streaming") or assert.IsType/require on the concrete type) and then
cast to listModel and continue the existing assertions (references:
WelcomeModel, tui.Model, listModel, isList, tui.ListState).
In `@internal/cli/commands/catalog/tui/welcome.go`:
- Around line 224-247: The handler currently falls through to welcomeNoSources
when discovery returns an error but no modules, hiding real failures; change
handleDiscoveryComplete to check msg.Err before treating the result as an empty
catalog: if msg.Err != nil, keep the existing log but set the model into an
error state (e.g., m.state = welcomeError or add a new welcomeDiscoveryError
constant) and save the error onto the model (e.g., m.discoveryErr or m.err) so
the TUI can render a distinct error screen, then return the updated model
without continuing to NewModel or setting welcomeNoSources; ensure
DiscoveryCompleteMsg handling uses these fields so real auth/network/config
failures are shown instead of the no-sources screen.
🪄 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: 928cbc02-39de-4e6c-95cc-dfb354145fda
📒 Files selected for processing (9)
internal/cli/commands/catalog/catalog_redesign.gointernal/cli/commands/catalog/tui/model.gointernal/cli/commands/catalog/tui/model_test.gointernal/cli/commands/catalog/tui/update.gointernal/cli/commands/catalog/tui/view.gointernal/cli/commands/catalog/tui/view_test.gointernal/cli/commands/catalog/tui/welcome.gointernal/cli/commands/catalog/tui/welcome_test.gointernal/services/catalog/catalog.go
Comment on lines
+75
to
+96
| loaders.Go(func() error { | ||
| if err := svc.LoadStreamingURL(loadCtx, l, repoURL, onModule); err != nil { | ||
| l.Warnf("Error loading %s: %v", repoURL, err) | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| } | ||
|
|
||
| if err := loaders.Wait(); err != nil { | ||
| l.Warnf("Loader error: %v", err) | ||
| } | ||
|
|
||
| if err := g.Wait(); err != nil { | ||
| l.Warnf("Discovery error: %v", err) | ||
| } | ||
|
|
||
| if len(svc.Modules()) == 0 { | ||
| return nil, nil | ||
| } | ||
|
|
||
| return svc, nil |
Contributor
There was a problem hiding this comment.
Don't convert loader failures into "no sources found".
Every LoadStreamingURL failure is downgraded to a warning, and the callback returns nil, nil when no modules were loaded. If all repos fail to clone/load, the welcome flow will present an empty-catalog state instead of surfacing the actual failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/commands/catalog/catalog_redesign.go` around lines 75 - 96, The
code currently swallows errors from LoadStreamingURL (inside loaders.Go) and
converts all failures into warnings and ultimately a nil, nil return when
svc.Modules() is empty; change the anonymous function passed to loaders.Go to
return the error from svc.LoadStreamingURL instead of only logging it so
loaders.Wait() can surface failures, and then propagate errors from
loaders.Wait() and g.Wait() by returning them (rather than only l.Warnf) so
callers of this function receive real errors instead of seeing an empty catalog;
ensure references: the anonymous func using svc.LoadStreamingURL(loadCtx, l,
repoURL, onModule), loaders.Wait(), g.Wait(), and the svc.Modules() empty check
are updated accordingly.
Comment on lines
+287
to
+302
| // After streaming completes, should have transitioned to Model | ||
| listModel, isList := m.(tui.Model) | ||
| if isList { | ||
| assert.Equal(t, tui.ListState, listModel.State, "should be in list state") | ||
|
|
||
| items := listModel.List.Items() | ||
| assert.GreaterOrEqual(t, len(items), 1, "should have at least one module in the list") | ||
|
|
||
| // Verify alphabetical order if multiple items present | ||
| for i := 1; i < len(items); i++ { | ||
| prev := items[i-1].(*module.Module).Title() | ||
| curr := items[i].(*module.Module).Title() | ||
| assert.LessOrEqual(t, strings.ToLower(prev), strings.ToLower(curr), | ||
| "modules should be in alphabetical order: %q should come before %q", prev, curr) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Make the streaming transition assertion mandatory.
If the model never leaves WelcomeModel, this test still passes because all of the list checks are gated behind if isList. That turns a broken streaming path into a false green.
Suggested fix
- listModel, isList := m.(tui.Model)
- if isList {
- assert.Equal(t, tui.ListState, listModel.State, "should be in list state")
-
- items := listModel.List.Items()
- assert.GreaterOrEqual(t, len(items), 1, "should have at least one module in the list")
-
- // Verify alphabetical order if multiple items present
- for i := 1; i < len(items); i++ {
- prev := items[i-1].(*module.Module).Title()
- curr := items[i].(*module.Module).Title()
- assert.LessOrEqual(t, strings.ToLower(prev), strings.ToLower(curr),
- "modules should be in alphabetical order: %q should come before %q", prev, curr)
- }
- }
+ listModel, isList := m.(tui.Model)
+ require.True(t, isList, "should transition to module list after streaming completes")
+ assert.Equal(t, tui.ListState, listModel.State, "should be in list state")
+
+ items := listModel.List.Items()
+ assert.GreaterOrEqual(t, len(items), 1, "should have at least one module in the list")
+
+ // Verify alphabetical order if multiple items present
+ for i := 1; i < len(items); i++ {
+ prev := items[i-1].(*module.Module).Title()
+ curr := items[i].(*module.Module).Title()
+ assert.LessOrEqual(t, strings.ToLower(prev), strings.ToLower(curr),
+ "modules should be in alphabetical order: %q should come before %q", prev, curr)
+ }📝 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.
Suggested change
| // After streaming completes, should have transitioned to Model | |
| listModel, isList := m.(tui.Model) | |
| if isList { | |
| assert.Equal(t, tui.ListState, listModel.State, "should be in list state") | |
| items := listModel.List.Items() | |
| assert.GreaterOrEqual(t, len(items), 1, "should have at least one module in the list") | |
| // Verify alphabetical order if multiple items present | |
| for i := 1; i < len(items); i++ { | |
| prev := items[i-1].(*module.Module).Title() | |
| curr := items[i].(*module.Module).Title() | |
| assert.LessOrEqual(t, strings.ToLower(prev), strings.ToLower(curr), | |
| "modules should be in alphabetical order: %q should come before %q", prev, curr) | |
| } | |
| } | |
| // After streaming completes, should have transitioned to Model | |
| listModel, isList := m.(tui.Model) | |
| require.True(t, isList, "should transition to module list after streaming completes") | |
| assert.Equal(t, tui.ListState, listModel.State, "should be in list state") | |
| items := listModel.List.Items() | |
| assert.GreaterOrEqual(t, len(items), 1, "should have at least one module in the list") | |
| // Verify alphabetical order if multiple items present | |
| for i := 1; i < len(items); i++ { | |
| prev := items[i-1].(*module.Module).Title() | |
| curr := items[i].(*module.Module).Title() | |
| assert.LessOrEqual(t, strings.ToLower(prev), strings.ToLower(curr), | |
| "modules should be in alphabetical order: %q should come before %q", prev, curr) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/commands/catalog/tui/view_test.go` around lines 287 - 302, The
test currently gates all list assertions behind "if isList" so a failure to
transition from WelcomeModel can silently pass; replace the conditional check
with a mandatory assertion that the runtime model is a list (e.g.,
assert.True(t, isList, "expected model to transition to tui.Model list after
streaming") or assert.IsType/require on the concrete type) and then cast to
listModel and continue the existing assertions (references: WelcomeModel,
tui.Model, listModel, isList, tui.ListState).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
internal/cli/commands/catalog/tui/redesign/update.go (3)
116-118: Simplify nestedtea.Batchcall.The nested
tea.Batch(cmd, tea.Batch(cmds...))can be flattened for clarity. Sincetea.Batchaccepts variadic commands, you can combine them directly.♻️ Proposed simplification
// Append any commands from button bar initialization if len(cmds) > 0 { - return m, tea.Batch(cmd, tea.Batch(cmds...)) + return m, tea.Batch(append([]tea.Cmd{cmd}, cmds...)...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/tui/redesign/update.go` around lines 116 - 118, The return uses a nested tea.Batch call unnecessarily; simplify by passing cmd plus the cmds slice directly to tea.Batch rather than wrapping cmds in a second tea.Batch. Update the return in the branch that currently returns m, tea.Batch(cmd, tea.Batch(cmds...)) to return m, tea.Batch(cmd, cmds...) (preserving m and variables cmd and cmds and the use of tea.Batch).
22-121: Consider extracting sub-handlers to reduce cognitive complexity.Static analysis flagged this function with cognitive complexity of 48 (allowed: 15). While TUI update handlers often have complex switch structures, extracting the module selection logic (lines 39-104) into a separate helper could improve readability.
♻️ Potential extraction example
func handleModuleSelection(msg tea.KeyPressMsg, m Model) (Model, []tea.Cmd) { selectedModule, ok := m.List.SelectedItem().(*module.Module) if !ok { return m, nil } switch { case key.Matches(msg, m.delegateKeys.Choose): return handleChooseModule(m, selectedModule) case key.Matches(msg, m.delegateKeys.Scaffold): return handleScaffoldFromList(m, selectedModule) } return m, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/tui/redesign/update.go` around lines 22 - 121, The updateList function's module selection block is too complex; extract the logic that handles a selected module into a helper such as handleModuleSelection(msg tea.KeyPressMsg, m Model) (Model, []tea.Cmd) which inspects m.List.SelectedItem() as *module.Module and dispatches to two smaller helpers like handleChooseModule(m Model, selectedModule *module.Module) (Model, []tea.Cmd) and handleScaffoldFromList(m Model, selectedModule *module.Module) (Model, []tea.Cmd); have updateList call handleModuleSelection when a KeyPressMsg is received, merge returned cmds into cmds slice (and keep existing state mutations like m.selectedModule and m.State inside the dedicated handlers), and ensure the helpers return any tea.Cmds to be batched back in updateList.
123-191: Static analysis flags complexity; current structure is acceptable for TUI handlers.While SonarCloud flagged cognitive complexity of 29 (allowed: 15), the
updatePagerfunction handles distinct concerns (button bar updates, key actions, viewport events) that are logically grouped. The complexity is inherent to TUI state management. Consider this a lower-priority refactor opportunity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/tui/redesign/update.go` around lines 123 - 191, The SonarCloud cognitive complexity flag on updatePager is acceptable but to reduce complexity and make intent clearer, extract the key action handling into small helper functions: e.g., create handleChooseAction(m Model, msg tea.KeyMsg) (Model, tea.Cmd) to contain the inner switch that inspects currentAction (referencing m.activeButton, scaffoldBtn, viewSourceBtn, scaffoldModuleCmd, m.selectedModule), and create handlePagerShortcut(m Model, msg tea.KeyMsg) (Model, tea.Cmd) for the separate key.Matches branches (Scaffold, Quit). Keep the button bar update (m.buttonBar.Update) and viewport update (m.viewport.Update) in updatePager, call the new helpers from the corresponding cases, and return their results; this reduces cognitive complexity while preserving behavior.internal/cli/commands/catalog/tui/redesign/welcome_test.go (1)
233-280: Good streaming test, but order assertion could be more robust.The new
TestWelcomeStreamingModulestest validates the streaming flow effectively. However, the alphabetical order assertion at lines 275-278 uses direct string comparison (prev <= curr) while the model sorts case-insensitively. If test modules have mixed-case titles, this assertion could fail or pass incorrectly.Consider using case-insensitive comparison to match the model's sorting behavior:
♻️ Suggested improvement for consistency
// Verify alphabetical order items := listModel.List.Items() for i := 1; i < len(items); i++ { prev := items[i-1].(*module.Module).Title() curr := items[i].(*module.Module).Title() - assert.LessOrEqual(t, prev, curr, "modules should be in alphabetical order") + assert.LessOrEqual(t, strings.ToLower(prev), strings.ToLower(curr), "modules should be in case-insensitive alphabetical order") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/tui/redesign/welcome_test.go` around lines 233 - 280, The alphabetical-order assertion in TestWelcomeStreamingModules uses direct string comparison of module.Module.Title() but the UI sorts case-insensitively; update the check in the loop that iterates over listModel.List.Items() to compare titles in a case-insensitive way (e.g., normalize both prev and curr with strings.ToLower or an equivalent case-insensitive comparison) so the test matches redesign.NewWelcomeModel's sorting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/commands/catalog/tui/redesign/model_test.go`:
- Around line 186-190: The test currently compares module titles
case-sensitively in the loop over items, using
items[i-1].(*module.Module).Title() and items[i].(*module.Module).Title();
update the assertion to perform a case-insensitive comparison by normalizing
both prev and curr with strings.ToLower (matching the behavior in model.go where
sorting uses strings.ToLower at line 86) before calling assert.LessOrEqual so
the test reflects the actual sort behavior.
- Around line 80-107: The test currently calls os.MkdirAll and os.WriteFile
(e.g., creating dummyRepoDir, gitDir, refsDir, headsDir, remotesDir and writing
config, HEAD, refs and fakeCommitHash) without checking returned errors; update
each filesystem call in model_test.go to capture the error and fail the test
immediately (use t.Fatalf or a test helper like require.NoError) with a clear
message including the operation and path. Apply the same error-checking pattern
to the similar filesystem operations inside the switch block and the write at
the later location referenced in the comment so any setup failure is reported at
the exact call site.
In `@internal/cli/commands/catalog/tui/redesign/model.go`:
- Around line 144-178: The method insertModuleSorted calls mod.Title() without
validating mod; add a nil guard at the start of insertModuleSorted to return nil
immediately if mod == nil (before computing modTitle), so the subsequent calls
(mod.Title(), isDuplicate, m.List.InsertItem, m.List.Select) never panic; ensure
the early return preserves current behavior by not altering m.userNavigated or
list state and reference insertModuleSorted, mod.Title, isDuplicate,
m.List.InsertItem and m.List.Select when making the change.
In `@internal/cli/commands/catalog/tui/redesign/view.go`:
- Line 14: The constant infoLineStyle uses an invalid 5-digit hex string
"#1D252"; update the color literal to the correct 6-digit hex (likely "#1D252F")
in the lipgloss.NewStyle() call for infoLineStyle so it matches the
titleBackgroundColor pattern and compiles with a valid color value.
---
Nitpick comments:
In `@internal/cli/commands/catalog/tui/redesign/update.go`:
- Around line 116-118: The return uses a nested tea.Batch call unnecessarily;
simplify by passing cmd plus the cmds slice directly to tea.Batch rather than
wrapping cmds in a second tea.Batch. Update the return in the branch that
currently returns m, tea.Batch(cmd, tea.Batch(cmds...)) to return m,
tea.Batch(cmd, cmds...) (preserving m and variables cmd and cmds and the use of
tea.Batch).
- Around line 22-121: The updateList function's module selection block is too
complex; extract the logic that handles a selected module into a helper such as
handleModuleSelection(msg tea.KeyPressMsg, m Model) (Model, []tea.Cmd) which
inspects m.List.SelectedItem() as *module.Module and dispatches to two smaller
helpers like handleChooseModule(m Model, selectedModule *module.Module) (Model,
[]tea.Cmd) and handleScaffoldFromList(m Model, selectedModule *module.Module)
(Model, []tea.Cmd); have updateList call handleModuleSelection when a
KeyPressMsg is received, merge returned cmds into cmds slice (and keep existing
state mutations like m.selectedModule and m.State inside the dedicated
handlers), and ensure the helpers return any tea.Cmds to be batched back in
updateList.
- Around line 123-191: The SonarCloud cognitive complexity flag on updatePager
is acceptable but to reduce complexity and make intent clearer, extract the key
action handling into small helper functions: e.g., create handleChooseAction(m
Model, msg tea.KeyMsg) (Model, tea.Cmd) to contain the inner switch that
inspects currentAction (referencing m.activeButton, scaffoldBtn, viewSourceBtn,
scaffoldModuleCmd, m.selectedModule), and create handlePagerShortcut(m Model,
msg tea.KeyMsg) (Model, tea.Cmd) for the separate key.Matches branches
(Scaffold, Quit). Keep the button bar update (m.buttonBar.Update) and viewport
update (m.viewport.Update) in updatePager, call the new helpers from the
corresponding cases, and return their results; this reduces cognitive complexity
while preserving behavior.
In `@internal/cli/commands/catalog/tui/redesign/welcome_test.go`:
- Around line 233-280: The alphabetical-order assertion in
TestWelcomeStreamingModules uses direct string comparison of
module.Module.Title() but the UI sorts case-insensitively; update the check in
the loop that iterates over listModel.List.Items() to compare titles in a
case-insensitive way (e.g., normalize both prev and curr with strings.ToLower or
an equivalent case-insensitive comparison) so the test matches
redesign.NewWelcomeModel's sorting behavior.
🪄 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: 1f7fc76b-d0ab-43f4-a0af-a6e4d926b8c0
📒 Files selected for processing (16)
internal/cli/commands/catalog/catalog_redesign.gointernal/cli/commands/catalog/tui/delegate.gointernal/cli/commands/catalog/tui/keys.gointernal/cli/commands/catalog/tui/model.gointernal/cli/commands/catalog/tui/redesign/model.gointernal/cli/commands/catalog/tui/redesign/model_test.gointernal/cli/commands/catalog/tui/redesign/source_discovery.gointernal/cli/commands/catalog/tui/redesign/source_discovery_test.gointernal/cli/commands/catalog/tui/redesign/update.gointernal/cli/commands/catalog/tui/redesign/view.gointernal/cli/commands/catalog/tui/redesign/view_test.gointernal/cli/commands/catalog/tui/redesign/welcome.gointernal/cli/commands/catalog/tui/redesign/welcome_test.gointernal/cli/commands/catalog/tui/update.gointernal/cli/commands/catalog/tui/view.gointernal/cli/commands/catalog/tui/view_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/cli/commands/catalog/tui/redesign/source_discovery.go
- internal/cli/commands/catalog/tui/view_test.go
- internal/cli/commands/catalog/tui/redesign/view_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cli/commands/catalog/tui/view.go
- internal/cli/commands/catalog/tui/update.go
- internal/cli/commands/catalog/catalog_redesign.go
yhakbar
force-pushed
the
feat/supporting-streamed-components
branch
from
7b2dfde to
e4e1a08
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
internal/cli/commands/catalog/catalog_redesign.go (2)
37-39:⚠️ Potential issue | 🟠 MajorMake streamed sends abortable.
moduleCh <- modcan still block forever once the UI stops consuming or the buffer fills during quit/cancel. That leaves the loader goroutine stuck even though the program is shutting down. Use aselecton the load context so the send can be abandoned cleanly.Suggested fix
- onModule := func(mod *module.Module) { - moduleCh <- mod - } - urlCh := make(chan string, 10) //nolint:mnd g, gctx := errgroup.WithContext(ctx) @@ maxWorkers := max(1, min(opts.Parallelism, runtime.GOMAXPROCS(0))) loaders, loadCtx := errgroup.WithContext(gctx) loaders.SetLimit(maxWorkers) + + onModule := func(mod *module.Module) { + select { + case moduleCh <- mod: + case <-loadCtx.Done(): + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/catalog_redesign.go` around lines 37 - 39, The onModule closure currently does an unguarded send "moduleCh <- mod" which can block indefinitely; change the send to be abortable by selecting between sending to moduleCh and the loader context's Done channel (e.g., select { case moduleCh <- mod: case <-loadCtx.Done(): }). Update the onModule function (the closure named onModule) to use this select so the loader goroutine can abandon the send and return cleanly when the load context is canceled.
75-96:⚠️ Potential issue | 🟠 MajorSurface loader/discovery failures instead of returning an empty catalog.
These branches log and then return success, so clone/discovery failures collapse into the same
nil, nilresult as "no sources found." The caller then loses the real failure and shows the empty-catalog flow. LetLoadStreamingURL,loaders.Wait(), andg.Wait()propagate their errors back out ofloadFunc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/catalog_redesign.go` around lines 75 - 96, The loader/discovery errors are being logged and swallowed causing loadFunc to return (nil, nil) for real failures; change the goroutine so it returns the error from svc.LoadStreamingURL instead of always returning nil (so loaders.Wait can surface it), and replace the l.Warnf branches that check loaders.Wait() and g.Wait() with returning that error from loadFunc (i.e., return nil, err) rather than just logging; keep the existing len(svc.Modules()) == 0 check for the no-sources case, but ensure any non-nil errors from LoadStreamingURL, loaders.Wait(), or g.Wait() are propagated upward.internal/cli/commands/catalog/tui/redesign/welcome.go (1)
224-247:⚠️ Potential issue | 🟠 MajorDon't fall through to
welcomeNoSourceswhen discovery failed.If
msg.Erris non-nil and no modules were loaded, this still renders the empty-catalog help screen. That makes auth/network/config failures indistinguishable from a genuinely empty catalog. Short-circuit into a dedicated error state instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/tui/redesign/welcome.go` around lines 224 - 247, In handleDiscoveryComplete, avoid falling through to welcomeNoSources when discovery failed: when msg.Err != nil, capture the error (e.g., add a field on WelcomeModel like lastDiscoveryErr error), set m.state to a new error state (e.g., welcomeDiscoveryError) and return immediately (return m, nil) instead of continuing to the empty-catalog branch; add the new welcomeDiscoveryError state handling and surface lastDiscoveryErr in the rendering logic so auth/network/config failures are shown distinctly from an empty catalog.
🧹 Nitpick comments (1)
internal/cli/commands/catalog/tui/redesign/update.go (1)
22-121: Consider extracting the action branches out of these update handlers.Both handlers now mix key matching, markdown rendering, button-bar construction, browser launch, and state transitions in one place, which is why Sonar is flagging them for complexity. Pulling those branches into small helpers would make the streaming behavior easier to reason about and test. As per coding guidelines, "Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain."
Also applies to: 123-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/tui/redesign/update.go` around lines 22 - 121, The updateList handler is too large and mixes key matching, markdown rendering, button-bar construction, and state transitions; refactor by extracting the inner action branches into small helper functions to reduce complexity: move the markdown rendering and viewport setup into a helper like renderSelectedModuleViewport(selected *module.Module, m *Model) (tea.Cmd, error) that uses glamour and rendererErrCmd, move button-bar creation into buildPagerButtons(selected *module.Module, m *Model) (buttonbar.Model, []button, tea.Cmd) that encapsulates buttonbar.New and m.buttonBar.Init(), and move the scaffold flow into a helper handleScaffoldAction(selected *module.Module, m *Model) (tea.Model, tea.Cmd) that calls scaffoldModuleCmd and sets m.State = ScaffoldState; update updateList to only perform key matching and call these helpers (setting m.selectedModule and m.State = PagerState where appropriate) and return combined commands via tea.Batch to preserve existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/commands/catalog/tui/redesign/model.go`:
- Around line 164-198: The de-duplication in isDuplicate currently compares
module.Title() case-insensitively, which can collapse distinct modules that
share a title; update isDuplicate to compare a stable identity field on
*module.Module (e.g., source path, URL, or explicit ID) instead of Title(),
leaving Title() usage only for sorting; modify the function to fetch the stable
key from the existing module (and from the incoming modTitle source/struct) and
use that key (with appropriate normalization) for equality checks while keeping
the rest of the insertion logic (m.List.InsertItem, Select handling, and
userNavigated behavior) unchanged.
In `@internal/cli/commands/catalog/tui/redesign/update.go`:
- Around line 93-100: The scaffold key handler currently uses `break`, which
only exits the switch and allows the keypress to be forwarded to the
list/viewport update path; change those early `break` branches (where you check
`if m.SVC == nil`) to consume the message by returning the current model and a
nil command (e.g., `return m, nil`) so the keypress is not passed on; update
every occurrence that checks `m.SVC == nil` for the `delegateKeys.Scaffold`
handling (including the blocks around `ScaffoldState`/`scaffoldModuleCmd` and
the similar cases at the other occurrences referenced) to follow this pattern.
In `@internal/cli/commands/catalog/tui/update.go`:
- Around line 91-98: The scaffold key handler currently uses "break" which only
exits the switch and allows the same keypress to be forwarded to m.List.Update /
m.viewport.Update; change the early-exit when m.SVC == nil to consume the
keypress by returning immediately (e.g., return m, nil or return m,
tea.Cmd(nil)) so the key isn't processed further. Update the same pattern for
the other occurrences handling m.delegateKeys.Scaffold (the blocks that check
m.SVC and set m.State = ScaffoldState and call scaffoldModuleCmd) so all
scaffold keypresses are consumed when m.SVC is unavailable.
In `@internal/services/catalog/catalog.go`:
- Around line 263-267: Modules() currently returns the shared s.modules slice
after releasing s.mu, which can race with concurrent appends from
LoadStreamingURL(); modify Modules() to hold s.mu, make and return a shallow
copy of s.modules (e.g., copy to a new slice) while still under the lock, and
ensure any assignment performed by Load() to s.modules is also done while
holding s.mu so all reads/writes use the same mutex-protected copy semantics.
---
Duplicate comments:
In `@internal/cli/commands/catalog/catalog_redesign.go`:
- Around line 37-39: The onModule closure currently does an unguarded send
"moduleCh <- mod" which can block indefinitely; change the send to be abortable
by selecting between sending to moduleCh and the loader context's Done channel
(e.g., select { case moduleCh <- mod: case <-loadCtx.Done(): }). Update the
onModule function (the closure named onModule) to use this select so the loader
goroutine can abandon the send and return cleanly when the load context is
canceled.
- Around line 75-96: The loader/discovery errors are being logged and swallowed
causing loadFunc to return (nil, nil) for real failures; change the goroutine so
it returns the error from svc.LoadStreamingURL instead of always returning nil
(so loaders.Wait can surface it), and replace the l.Warnf branches that check
loaders.Wait() and g.Wait() with returning that error from loadFunc (i.e.,
return nil, err) rather than just logging; keep the existing len(svc.Modules())
== 0 check for the no-sources case, but ensure any non-nil errors from
LoadStreamingURL, loaders.Wait(), or g.Wait() are propagated upward.
In `@internal/cli/commands/catalog/tui/redesign/welcome.go`:
- Around line 224-247: In handleDiscoveryComplete, avoid falling through to
welcomeNoSources when discovery failed: when msg.Err != nil, capture the error
(e.g., add a field on WelcomeModel like lastDiscoveryErr error), set m.state to
a new error state (e.g., welcomeDiscoveryError) and return immediately (return
m, nil) instead of continuing to the empty-catalog branch; add the new
welcomeDiscoveryError state handling and surface lastDiscoveryErr in the
rendering logic so auth/network/config failures are shown distinctly from an
empty catalog.
---
Nitpick comments:
In `@internal/cli/commands/catalog/tui/redesign/update.go`:
- Around line 22-121: The updateList handler is too large and mixes key
matching, markdown rendering, button-bar construction, and state transitions;
refactor by extracting the inner action branches into small helper functions to
reduce complexity: move the markdown rendering and viewport setup into a helper
like renderSelectedModuleViewport(selected *module.Module, m *Model) (tea.Cmd,
error) that uses glamour and rendererErrCmd, move button-bar creation into
buildPagerButtons(selected *module.Module, m *Model) (buttonbar.Model, []button,
tea.Cmd) that encapsulates buttonbar.New and m.buttonBar.Init(), and move the
scaffold flow into a helper handleScaffoldAction(selected *module.Module, m
*Model) (tea.Model, tea.Cmd) that calls scaffoldModuleCmd and sets m.State =
ScaffoldState; update updateList to only perform key matching and call these
helpers (setting m.selectedModule and m.State = PagerState where appropriate)
and return combined commands via tea.Batch to preserve existing behavior.
🪄 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: 2e5c4215-12b7-44c2-941c-8e1a71a9b1e2
📒 Files selected for processing (17)
internal/cli/commands/catalog/catalog_redesign.gointernal/cli/commands/catalog/tui/delegate.gointernal/cli/commands/catalog/tui/keys.gointernal/cli/commands/catalog/tui/model.gointernal/cli/commands/catalog/tui/redesign/model.gointernal/cli/commands/catalog/tui/redesign/model_test.gointernal/cli/commands/catalog/tui/redesign/source_discovery.gointernal/cli/commands/catalog/tui/redesign/source_discovery_test.gointernal/cli/commands/catalog/tui/redesign/update.gointernal/cli/commands/catalog/tui/redesign/view.gointernal/cli/commands/catalog/tui/redesign/view_test.gointernal/cli/commands/catalog/tui/redesign/welcome.gointernal/cli/commands/catalog/tui/redesign/welcome_test.gointernal/cli/commands/catalog/tui/update.gointernal/cli/commands/catalog/tui/view.gointernal/cli/commands/catalog/tui/view_test.gointernal/services/catalog/catalog.go
✅ Files skipped from review due to trivial changes (4)
- internal/cli/commands/catalog/tui/redesign/source_discovery.go
- internal/cli/commands/catalog/tui/redesign/source_discovery_test.go
- internal/cli/commands/catalog/tui/redesign/view_test.go
- internal/cli/commands/catalog/tui/view_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/cli/commands/catalog/tui/delegate.go
- internal/cli/commands/catalog/tui/keys.go
- internal/cli/commands/catalog/tui/model.go
- internal/cli/commands/catalog/tui/redesign/model_test.go
- internal/cli/commands/catalog/tui/view.go
yhakbar
force-pushed
the
feat/supporting-streamed-components
branch
from
e4e1a08 to
601f9b1
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/services/catalog/catalog.go (1)
264-268:⚠️ Potential issue | 🟠 MajorProtect
Load()'s assignment with the same mutex.
Modules()now clones unders.mu, butLoad()still replacess.moduleswithout locking at Line 203. That leaves a write racing withModules()andLoadStreamingURL(), so the snapshot guarantee is still incomplete.🔒 Suggested fix
- s.modules = allModules + s.mu.Lock() + s.modules = allModules + s.mu.Unlock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/services/catalog/catalog.go` around lines 264 - 268, The Modules() method now clones s.modules under s.mu but Load() still assigns s.modules without locking, causing a race; update the Load() function to acquire s.mu (e.g., s.mu.Lock() / defer s.mu.Unlock()) around the assignment to s.modules (or build the new slice off-lock and then lock only to swap the pointer) so that writes to s.modules are protected by the same mutex used in Modules() and LoadStreamingURL().internal/cli/commands/catalog/catalog_redesign.go (1)
79-99:⚠️ Potential issue | 🟠 MajorPropagate real discovery/load failures to the welcome flow.
These branches log and continue, and Lines 98-99 then return
nil, nilwhen nothing loaded. That means malformed catalog config, source-discovery failures, or "every repo failed to load" still render as the empty-catalog screen instead of the new discovery-error screen ininternal/cli/commands/catalog/tui/redesign/welcome.go.Return an error when discovery/loading fails and no modules were produced, or aggregate partial failures and surface them through
DiscoveryCompleteMsg.Also applies to: 111-115, 133-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/catalog_redesign.go` around lines 79 - 99, The current logic swallows discovery/load failures and returns nil,nil when svc.Modules() is empty, causing the welcome flow to show an empty catalog instead of surfacing discovery errors; change the control flow in the catalog loading routine so that when loaders.Wait() or g.Wait() returns an error or when all repo loads fail and svc.Modules() is empty you return that error (or an aggregated error of individual repo failures from svc.LoadStreamingURL) instead of nil,nil, and pass aggregated failure details into the DiscoveryCompleteMsg path so the welcome UI (redesign/welcome.go) can render the discovery-error screen; update places around svc.LoadStreamingURL, loaders.Wait, g.Wait and the svc.Modules() check to propagate/aggregate errors rather than silently continuing.
🧹 Nitpick comments (2)
internal/cli/commands/catalog/tui/redesign/update.go (2)
123-190: BreakupdatePagerinto focused action helpers.
updatePageris doing button-bar updates, pager action dispatch, browser launching, quit handling, and viewport updates in one place. Extracting the scaffold/view-source/quit branches will reduce the current complexity and make regressions easier to isolate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/tui/redesign/update.go` around lines 123 - 190, The updatePager function is doing too many things; split its responsibilities into focused helper functions to improve readability and testability: extract the Choose branch logic into a helper like handlePagerChoose(m *Model, msg tea.KeyMsg) (tea.Model, tea.Cmd) that reads m.activeButton/currentPagerButtons, handles scaffold (calling scaffoldModuleCmd), viewSource (opening URL and setting viewport content on error), and default logging; extract the Scaffold key branch into handlePagerScaffold(m *Model) (tea.Model, tea.Cmd) that checks m.SVC, sets m.State to ScaffoldState and returns scaffoldModuleCmd; extract the Quit branch into handlePagerQuit(m *Model) (tea.Model, tea.Cmd) that sets m.State = ListState; and extract viewport update into updateViewport(m Model, msg tea.Msg) (Model, tea.Cmd) which updates m.viewport and returns the cmd; then call these helpers from updatePager in place of the large switch, keeping existing symbols like updatePager, scaffoldModuleCmd, m.buttonBar, m.activeButton, m.currentPagerButtons, and m.viewport to locate code.
22-120: SplitupdateListinto smaller handlers.This function now owns list key routing, markdown rendering, pager setup, button-bar creation, and scaffold dispatch. Pulling the choose/scaffold paths into dedicated helpers would make the streaming flow much easier to reason about and test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/catalog/tui/redesign/update.go` around lines 22 - 120, The updateList function is doing too many things; split the choose/scaffold branches into smaller helpers: create handleChooseKey(msg tea.KeyPressMsg, m Model) (Model, []tea.Cmd) to encapsulate markdown rendering (extract renderModuleContent(selectedModule, width) (string, error)), viewport setup, pager button creation (extract setupPagerButtons(selectedModule) (buttonbar.ButtonBar, []button, []string, []tea.Cmd) or similar), and state transitions; and create handleScaffoldKey(msg tea.KeyPressMsg, m Model, selectedModule *module.Module) (Model, tea.Cmd) to handle SVC checks and return scaffoldModuleCmd. Update updateList to call these helpers from the key switch, propagate returned cmds by appending to cmds or returning directly, and keep only routing and m.List.Update in updateList so flow is easier to test and reason about.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/cli/commands/catalog/catalog_redesign.go`:
- Around line 79-99: The current logic swallows discovery/load failures and
returns nil,nil when svc.Modules() is empty, causing the welcome flow to show an
empty catalog instead of surfacing discovery errors; change the control flow in
the catalog loading routine so that when loaders.Wait() or g.Wait() returns an
error or when all repo loads fail and svc.Modules() is empty you return that
error (or an aggregated error of individual repo failures from
svc.LoadStreamingURL) instead of nil,nil, and pass aggregated failure details
into the DiscoveryCompleteMsg path so the welcome UI (redesign/welcome.go) can
render the discovery-error screen; update places around svc.LoadStreamingURL,
loaders.Wait, g.Wait and the svc.Modules() check to propagate/aggregate errors
rather than silently continuing.
In `@internal/services/catalog/catalog.go`:
- Around line 264-268: The Modules() method now clones s.modules under s.mu but
Load() still assigns s.modules without locking, causing a race; update the
Load() function to acquire s.mu (e.g., s.mu.Lock() / defer s.mu.Unlock()) around
the assignment to s.modules (or build the new slice off-lock and then lock only
to swap the pointer) so that writes to s.modules are protected by the same mutex
used in Modules() and LoadStreamingURL().
---
Nitpick comments:
In `@internal/cli/commands/catalog/tui/redesign/update.go`:
- Around line 123-190: The updatePager function is doing too many things; split
its responsibilities into focused helper functions to improve readability and
testability: extract the Choose branch logic into a helper like
handlePagerChoose(m *Model, msg tea.KeyMsg) (tea.Model, tea.Cmd) that reads
m.activeButton/currentPagerButtons, handles scaffold (calling
scaffoldModuleCmd), viewSource (opening URL and setting viewport content on
error), and default logging; extract the Scaffold key branch into
handlePagerScaffold(m *Model) (tea.Model, tea.Cmd) that checks m.SVC, sets
m.State to ScaffoldState and returns scaffoldModuleCmd; extract the Quit branch
into handlePagerQuit(m *Model) (tea.Model, tea.Cmd) that sets m.State =
ListState; and extract viewport update into updateViewport(m Model, msg tea.Msg)
(Model, tea.Cmd) which updates m.viewport and returns the cmd; then call these
helpers from updatePager in place of the large switch, keeping existing symbols
like updatePager, scaffoldModuleCmd, m.buttonBar, m.activeButton,
m.currentPagerButtons, and m.viewport to locate code.
- Around line 22-120: The updateList function is doing too many things; split
the choose/scaffold branches into smaller helpers: create handleChooseKey(msg
tea.KeyPressMsg, m Model) (Model, []tea.Cmd) to encapsulate markdown rendering
(extract renderModuleContent(selectedModule, width) (string, error)), viewport
setup, pager button creation (extract setupPagerButtons(selectedModule)
(buttonbar.ButtonBar, []button, []string, []tea.Cmd) or similar), and state
transitions; and create handleScaffoldKey(msg tea.KeyPressMsg, m Model,
selectedModule *module.Module) (Model, tea.Cmd) to handle SVC checks and return
scaffoldModuleCmd. Update updateList to call these helpers from the key switch,
propagate returned cmds by appending to cmds or returning directly, and keep
only routing and m.List.Update in updateList so flow is easier to test and
reason about.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 045fdefc-fe00-429b-bd90-f5236e814acc
📒 Files selected for processing (11)
internal/cli/commands/catalog/catalog_redesign.gointernal/cli/commands/catalog/tui/keys.gointernal/cli/commands/catalog/tui/redesign/model.gointernal/cli/commands/catalog/tui/redesign/model_test.gointernal/cli/commands/catalog/tui/redesign/update.gointernal/cli/commands/catalog/tui/redesign/view.gointernal/cli/commands/catalog/tui/redesign/welcome.gointernal/cli/commands/catalog/tui/redesign/welcome_test.gointernal/cli/commands/catalog/tui/update.gointernal/cli/commands/catalog/tui/view_test.gointernal/services/catalog/catalog.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cli/commands/catalog/tui/update.go
- internal/cli/commands/catalog/tui/redesign/welcome_test.go
- internal/cli/commands/catalog/tui/keys.go
yhakbar
force-pushed
the
feat/supporting-streamed-components
branch
from
601f9b1 to
8ad180b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/commands/catalog/tui/redesign/update.go`:
- Around line 37-118: The action branches handling m.delegateKeys.Choose and
m.delegateKeys.Scaffold (and similarly the Choose/ViewSource handling around
lines ~140-190) must consume the keypress and return immediately so the same msg
does not fall through into m.List.Update or m.viewport.Update; after preparing
the viewport, setting m.State (PagerState/ScaffoldState), initializing
m.buttonBar and appending its Init() cmd, replace the current fall-through logic
with an immediate return of m plus either tea.Batch(append([]tea.Cmd{cmd},
cmds...)...) or the scaffoldModuleCmd result as appropriate (e.g., when invoking
scaffoldModuleCmd(m.logger, m, m.SVC, selectedModule) return that cmd
immediately), ensuring only the intended cmds are batched and no further
m.List.Update(msg) or m.viewport.Update(msg) runs for that handled key.
- Around line 228-233: The scaffoldFinishedMsg handler discards the tea.Printf
command so scaffold errors never display; update the case for
scaffoldFinishedMsg to return both the print and quit commands together by
replacing the lone tea.Quit return with a batched command using
tea.Batch(tea.Printf("error scaffolding module: %s", msg.err.Error()), tea.Quit)
when msg.err != nil, otherwise return m, tea.Quit; ensure you reference the
scaffoldFinishedMsg case, msg.err, tea.Printf, tea.Quit and tea.Batch when
making the change.
In `@internal/cli/commands/catalog/tui/redesign/view_test.go`:
- Around line 169-181: The test is manually invoking the batched command
returned by WelcomeModel.Init(), which bypasses tea.Program's batch handling;
update the test to run the model through the real Tea event loop by using the
existing runTeaModel() helper (from welcome_test.go) or, if you prefer
unit-level assertions, explicitly unwrap the tea.Batch returned by
WelcomeModel.Init() and invoke each inner command (m.spinner.Tick,
m.startDiscovery, m.listenForStatus, m.listenForModule) and feed their returned
messages into the model's Update loop; replace the cmd() goroutine block with a
call to runTeaModel(m) or with logic that iterates the Batch commands and passes
their messages into m.Update so the test exercises actual program semantics.
In `@internal/cli/commands/catalog/tui/redesign/welcome.go`:
- Around line 216-223: The code is currently synthesizing tea.WindowSizeMsg
using m.width/m.height even when those are zero, which initializes the next
model with invalid inner dimensions; update the logic around creating the batch
that returns tea.WindowSizeMsg so it only enqueues the synthetic WindowSizeMsg
when both m.width and m.height are > 0 (i.e., check width, height := m.width,
m.height and conditionally include the func returning tea.WindowSizeMsg{Width:
width, Height: height} in the tea.Batch only when width>0 && height>0). Apply
the same guarded check to the other transition path that constructs a newModel
and returns a tea.Batch (the similar block later in the file) so neither path
forwards a zero-sized WindowSizeMsg.
🪄 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: 25d75e7d-c6a8-45eb-9a82-65df26d43498
📒 Files selected for processing (17)
internal/cli/commands/catalog/catalog_redesign.gointernal/cli/commands/catalog/tui/delegate.gointernal/cli/commands/catalog/tui/keys.gointernal/cli/commands/catalog/tui/model.gointernal/cli/commands/catalog/tui/redesign/model.gointernal/cli/commands/catalog/tui/redesign/model_test.gointernal/cli/commands/catalog/tui/redesign/source_discovery.gointernal/cli/commands/catalog/tui/redesign/source_discovery_test.gointernal/cli/commands/catalog/tui/redesign/update.gointernal/cli/commands/catalog/tui/redesign/view.gointernal/cli/commands/catalog/tui/redesign/view_test.gointernal/cli/commands/catalog/tui/redesign/welcome.gointernal/cli/commands/catalog/tui/redesign/welcome_test.gointernal/cli/commands/catalog/tui/update.gointernal/cli/commands/catalog/tui/view.gointernal/cli/commands/catalog/tui/view_test.gointernal/services/catalog/catalog.go
✅ Files skipped from review due to trivial changes (3)
- internal/cli/commands/catalog/tui/redesign/source_discovery.go
- internal/cli/commands/catalog/tui/redesign/source_discovery_test.go
- internal/cli/commands/catalog/tui/view_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/cli/commands/catalog/tui/delegate.go
- internal/cli/commands/catalog/tui/model.go
- internal/cli/commands/catalog/tui/redesign/welcome_test.go
- internal/cli/commands/catalog/catalog_redesign.go
- internal/services/catalog/catalog.go
- internal/cli/commands/catalog/tui/keys.go
- internal/cli/commands/catalog/tui/redesign/model.go
yhakbar
force-pushed
the
feat/supporting-streamed-components
branch
from
8ad180b to
552aa8b
Compare
denis256
approved these changes
Apr 17, 2026
|
|
||
| return m, scaffoldModuleCmd(m.logger, m, m.SVC, m.selectedModule) | ||
| case viewSourceBtn: | ||
| if m.selectedModule.URL() != "" { |
Member
There was a problem hiding this comment.
Can be nil dereference in m.selectedModule.URL()?
Collaborator
Author
There was a problem hiding this comment.
Na, but we could add some hardening in the future to guarantee it doesn't change.
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.
Description
Makes it so that the catalog gets modules streamed in instead of waiting for them to be fully loaded before rendering the catalog TUI.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Bug Fixes
Tests