Skip to content

Fix MapSet assign summary updates#988

Merged
GuzekAlan merged 6 commits into
software-mansion:mainfrom
dl-alexandre:codex/fix-mapset-assign-summary
May 27, 2026
Merged

Fix MapSet assign summary updates#988
GuzekAlan merged 6 commits into
software-mansion:mainfrom
dl-alexandre:codex/fix-mapset-assign-summary

Conversation

@dl-alexandre
Copy link
Copy Markdown
Contributor

Summary

  • Carry the full updated struct through struct diffs so display nodes can refresh value-dependent collapsed content.
  • Refresh struct display content while preserving existing child diffs and expanded state.
  • Add a regression test for a MapSet assign whose collapsed summary changes after a diff update.

Fixes #986

Validation

  • mix format --check-formatted lib/live_debugger/app/utils/term_differ.ex lib/live_debugger/app/utils/term_parser.ex test/app/utils/term_differ_test.exs test/app/utils/term_parser_test.exs
  • mix test test/app/utils/term_differ_test.exs test/app/utils/term_parser_test.exs test/app/debugger/node_state/queries_test.exs test/app/debugger/node_state/web/components_test.exs
  • mix test

@dl-alexandre dl-alexandre changed the title [codex] Fix MapSet assign summary updates Fix MapSet assign summary updates May 10, 2026
@kraleppa kraleppa requested review from GuzekAlan and Copilot May 14, 2026 08:38
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bug where collapsed display of a struct (e.g., MapSet) was not refreshed after a diff update, so users saw the original inspect output even after the value changed. The fix carries the full updated/old struct in the :struct diff and uses it to regenerate the struct's collapsed content while preserving children diffs and expansion state.

Changes:

  • TermDiffer.diff/2 for structs now populates :ins/:del with the new and old struct under the primitive_key.
  • TermParser.update_by_diff!/3 for :struct now refreshes the node's content/kind/key/expanded markers from the new struct via a new refresh_struct_content/3 helper, before applying child diffs.
  • Adds tests covering the new diff shape and the collapsed-content refresh (including a MapSet regression case).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
lib/live_debugger/app/utils/term_differ.ex Includes full old/new struct in :struct diff via ins/del.
lib/live_debugger/app/utils/term_parser.ex Regenerates struct display content from updated struct while preserving children/state.
test/app/utils/term_differ_test.exs Updates struct diff expectation to include ins/del.
test/app/utils/term_parser_test.exs Adds regression test for MapSet collapsed content update.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/live_debugger/app/utils/term_differ.ex Outdated
Comment thread lib/live_debugger/app/utils/term_parser.ex Outdated
- TermDiffer.diff/2 for structs no longer carries `del` — the struct
  refresh in TermParser only reads `ins`, so retaining the old struct
  was dead weight and extra memory (per @GuzekAlan).
- TermParser uses a `@primitive_key` module attribute instead of
  calling TermDiffer.primitive_key/0 per invocation (per @GuzekAlan).
- Update term_differ_test struct-diff expectation to match.
@dl-alexandre
Copy link
Copy Markdown
Contributor Author

Thanks for the review @GuzekAlan — both points addressed in 4ca82c9:

  • term_differ.ex — dropped del from the :struct diff. TermParser.refresh_struct_content/3 only reads ins, so carrying the old struct was unused memory. Kept the :struct shape minimal (ins + diff).
  • term_parser.exprimitive_key is now a @primitive_key module attribute instead of a per-call TermDiffer.primitive_key/0.
  • Updated the term_differ_test.exs struct-diff expectation to match.

Compiles clean with --warnings-as-errors.

@dl-alexandre dl-alexandre marked this pull request as ready for review May 14, 2026 17:07
Comment thread lib/live_debugger/app/utils/term_parser.ex Outdated
Comment thread lib/live_debugger/app/utils/term_parser.ex Outdated
…summary_node to avoid unnecessary children recalc
@dl-alexandre
Copy link
Copy Markdown
Contributor Author

Addressed the latest review comments:

  • Refactored refresh_struct_content/3 to use direct pattern matching on the %Diff{ins: %{@primitive_key => struct}} clause (plus a catch-all), eliminating the nested case Map.fetch/2.

  • Extracted struct_summary_node/1 which builds a TermNode containing only the collapsed content/expanded_before/expanded_after/kind (with empty children) so that summary refreshes for structs like MapSet no longer unnecessarily recalculate child nodes via to_key_value_children/1.

  • Extracted key_prefix_and_separator/1 to share the key-label logic and avoid duplication.

  • mix format, targeted tests (term_*_test.exs, node_state tests), and --warnings-as-errors all pass.

Ready for re-review / merge. This keeps the child diff/expansion state intact while fixing the collapsed summary for changing MapSet (and other custom-Inspect structs) assigns.

Comment thread lib/live_debugger/app/utils/term_parser.ex
Comment thread test/app/utils/term_parser_test.exs Outdated
Comment thread lib/live_debugger/app/utils/term_parser.ex
@GuzekAlan GuzekAlan merged commit 1baaf56 into software-mansion:main May 27, 2026
4 checks passed
GuzekAlan added a commit that referenced this pull request May 27, 2026
* fix MapSet assign summary updates

* Update lib/live_debugger/app/utils/term_parser.ex

---------

Co-authored-by: Alan Guzek <alanguzek@gmail.com>
GuzekAlan added a commit that referenced this pull request May 27, 2026
* fix MapSet assign summary updates

* Update lib/live_debugger/app/utils/term_parser.ex

---------

Co-authored-by: Alan Guzek <alanguzek@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MapSet struct doesn't update

3 participants