fix(model): route huh's own messages so the export popup can submit#51
fix(model): route huh's own messages so the export popup can submit#51kanywst wants to merge 1 commit into
Conversation
Update only handed tea.KeyPressMsg to the export form, and huh advances through messages it emits as commands: pressing enter in a field produces a nextFieldMsg, and Form.Update only reaches StateCompleted once it receives a nextGroupMsg. Those came back into Update as plain tea.Msg values, matched no case in the type switch, and were dropped. The form therefore sat on the Filename field forever. StateCompleted was never reached, handleExportCommand was never called, and esc was the only way out -- e was a dead key. Route any message to the form while the export popup is open, and forward WindowSizeMsg to it as well so it sizes itself. The existing export tests call handleExportCommand directly, so they passed throughout. Add one that drives the real key path instead, pumping commands back through Update the way the runtime does.
📝 WalkthroughWalkthrough
ChangesExport form flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Model.Update
participant huhExportForm
participant handleExportCommand
participant Filesystem
User->>Model.Update: submit export form input
Model.Update->>huhExportForm: forward message
huhExportForm-->>Model.Update: completed filename and format
Model.Update->>handleExportCommand: invoke export
handleExportCommand->>Filesystem: write exported file
Filesystem-->>Model.Update: export result
Model.Update-->>User: show success alert
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/model/update.go`:
- Around line 76-82: Update the SplashDoneMsg handling in the model update flow
so it cannot reset viewMode while exportFormOpen() is true. Route the message
through updateExportForm or otherwise ignore it after splash dismissal,
preserving the active PopupExport state and populated exportForm.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: feed1574-7867-4fe8-9d58-b24adc22d55c
📒 Files selected for processing (2)
internal/model/update.gointernal/model/update_test.go
| // huh drives the export form through its own messages (nextFieldMsg, | ||
| // nextGroupMsg, ...), which it returns as commands. They come back here as | ||
| // plain tea.Msg values, so the form only ever advances a field or reaches | ||
| // StateCompleted if we hand them back to it. | ||
| if m.exportFormOpen() { | ||
| return m.updateExportForm(msg) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file structure first.
ast-grep outline internal/model/update.go --view expanded
# Read the relevant slice with line numbers.
sed -n '1,220p' internal/model/update.go | cat -n
# Search for export form message routing and completion helpers.
rg -n "exportFormOpen|updateExportForm|SplashDoneMsg|MouseWheelMsg|nextFieldMsg|nextGroupMsg|StateCompleted|ViewNormal|Ctrl\\+C|resize|WindowSizeMsg" internal/model/update.goRepository: kanywst/y509
Length of output: 9547
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect popup handling and any splash-related message source.
sed -n '220,340p' internal/model/update.go | cat -n
rg -n "type SplashDoneMsg|SplashDoneMsg|newExportForm|PopupExport|viewMode = ViewNormal|updatePopupMode|exportForm" internal/model -g '!**/*_test.go'
# Read any file that defines SplashDoneMsg if present.
fd -a "SplashDoneMsg" internal || trueRepository: kanywst/y509
Length of output: 6255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the splash command path and any mode transitions around it.
sed -n '240,310p' internal/model/command.go | cat -n
sed -n '1,120p' internal/model/constants.go | cat -n
sed -n '1,220p' internal/model/model.go | cat -n
# Look for any code that opens the export popup during splash or before splash completion.
rg -n "ViewSplash|SplashDoneMsg|viewMode = ViewSplash|PopupExport|exportFormOpen|PopupNone|Init\\(" internal/modelRepository: kanywst/y509
Length of output: 17106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the init path around the splash command.
sed -n '268,290p' internal/model/model.go | cat -n
# Inspect any tests or code that imply splash dismissal does not cancel the delayed splash message.
rg -n "SplashDoneMsg|ViewSplash|tea.Tick|tea.Sequence|dismiss.*splash|any key" internal/model/model_test.go internal/model/update_test.go internal/model/logic_test.go internal/modelRepository: kanywst/y509
Length of output: 2193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the popup rendering conditions.
sed -n '650,710p' internal/model/view.go | cat -n
# Inspect the export popup tests and any state assertions around popup visibility.
sed -n '150,250p' internal/model/update_test.go | cat -n
sed -n '200,260p' internal/model/logic_test.go | cat -nRepository: kanywst/y509
Length of output: 6276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the top-level View routing around popups.
sed -n '1,120p' internal/model/view.go | cat -n
# Show any tests that exercise splash-to-normal or popup rendering transitions.
rg -n "renderPopup\\(|ViewPopup|ViewNormal|SplashDoneMsg|PopupExport" internal/model/*_test.goRepository: kanywst/y509
Length of output: 5964
Keep SplashDoneMsg from closing an active export popup.
A delayed splash-timer message can still arrive after splash has been dismissed and the export form is open; this resets viewMode to ViewNormal while leaving popupType == PopupExport and exportForm populated. Route SplashDoneMsg through the popup path when exportFormOpen() is true, or ignore it once splash has been exited.
🤖 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 `@internal/model/update.go` around lines 76 - 82, Update the SplashDoneMsg
handling in the model update flow so it cannot reset viewMode while
exportFormOpen() is true. Route the message through updateExportForm or
otherwise ignore it after splash dismissal, preserving the active PopupExport
state and populated exportForm.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the export form handling in the Bubble Tea model by extracting the update logic into a helper function and ensuring it intercepts window size and general messages when the form is open. It also adds a comprehensive integration test using a custom message pump to simulate the runtime behavior. The review feedback points out a potential issue where a failed type assertion could lead to checking the state of a stale form, and suggests handling this case gracefully.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| form, cmd := m.exportForm.Update(msg) | ||
| if f, ok := form.(*huh.Form); ok { | ||
| m.exportForm = f | ||
| } | ||
|
|
||
| if m.exportForm.State != huh.StateCompleted { | ||
| return m, cmd | ||
| } |
There was a problem hiding this comment.
If the type assertion form.(*huh.Form) fails (ok is false), m.exportForm will not be updated to the new form state. However, the code still proceeds to check m.exportForm.State on the stale/old form instance on line 100. If ok is false, we should handle it gracefully (e.g., by returning early) to avoid checking the state of a stale form.
| form, cmd := m.exportForm.Update(msg) | |
| if f, ok := form.(*huh.Form); ok { | |
| m.exportForm = f | |
| } | |
| if m.exportForm.State != huh.StateCompleted { | |
| return m, cmd | |
| } | |
| f, ok := form.(*huh.Form) | |
| if !ok { | |
| return m, cmd | |
| } | |
| m.exportForm = f | |
| if m.exportForm.State != huh.StateCompleted { | |
| return m, cmd | |
| } |
1 participant
Problem
The export popup (
e) can never be submitted. It is a dead key —escis the only way out.Update's type switch only routestea.KeyPressMsgto the export form. But huh is message-driven: pressing enter in a field makesGroup.Updatereturn a command, which produces anextFieldMsg/nextGroupMsg, andForm.Updateonly reachesStateCompletedonce it receives anextGroupMsg. Those messages came back intoUpdateas plaintea.Msgvalues, matched no case, and fell through toreturn m, nil.So the form stays on the Filename field forever, focus never moves to Format,
StateCompletednever fires, andhandleExportCommandis never called. TheStateCompletedblock inupdatePopupModewas unreachable code.Driving the real event loop before this change:
Fix
Extract the form handling into
updateExportFormand route any message to it while the export popup is open, not just key presses.tea.WindowSizeMsgis forwarded too, so the form sizes itself.Why this wasn't caught
Every existing export test calls
handleExportCommanddirectly, bypassingUpdateentirely — so they passed the whole time. This addsTestExportFormCompletesThroughUpdate, which pressese, types a filename, and confirms both fields, pumping returned commands back throughUpdatethe way the Bubble Tea runtime does. Verified it fails against the old routing:make lintclean, full suite green.Summary by CodeRabbit
Bug Fixes
Tests