Fix bugs, add tests, and simplify WebSocket refactor - #436
Merged
nlef merged 8 commits intoApr 7, 2026
Conversation
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
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
Found several bugs while reviewing the refactor and added integration tests that would have caught them. Also reverted some abstractions that made things worse rather than better.
Bug fixes
set_timelapse_params/set_notify_paramsdrop the first parameter — the prefix is stripped by the caller, then the downstream method doespop(0)on what's left, eating the first real param. Fixed by removingpop(0)so they just accept params directly._update_print_stats_from_messageuses walrus operatorif val := dict.get(key)which silently skips zero/empty values (e.g.filament_used=0at print start,filename=""). Replaced withif "key" in dictguards to handle any value including falsy ones.status_responsetriggers height notifications and timelapse photos on reconnect after the unification withnotify_status_update— on initial connection these are state snapshots, not real Z moves.Why dispatch tables were reverted
The dispatch tables lose all type info — everything is
Callable[..., Any], so mypy can't check anything. The lambdas needsetattr/getattrhacks for simple assignments._PRINT_STATE_CONFIGSalso introduced behavioral regressions: paused state was settingtimelapse.is_running = False(original never touched it), and error/cancelled/standby were gatingis_running = Falsebehindmanual_mode(original stopped timelapse unconditionally on these). The per-state logic is too conditional for a flat config — different ordering, nested branches, some states gate behindmanual_modeand others don't. Reverted to the original if/elif which is explicit and mypy can actually check.For the gcode command dispatch, used
str.partition(" ")to split command from payload, then if/elif on the exact command word. No more prefix ordering issues ("tgnotify"matching before"tgnotify_photo"), no string duplication, no lambdas.Kept the sensor prefix tuples and klippy reconnect states — those are just data, no
Callabletypes involved.Other changes
KlippyStateandPrintStateare nowstr, Enumso comparisons with JSON strings are type-safeNOTIFY_START/NOTIFY_FINISHto make it obvious they don't come from KlipperPAUSEDandCOMPLETEtoPrintState(Klipper sends these but they were missing)