feat(own): fold cleanup into OnClosed + wider lambda delegates - #5
Conversation
Quality improvement for the subscription/disposable-field shapes: when the owner
class already has a 'protected override void OnClosed(...)' (which runs exactly
when the Window's Closed event would), insert the detach/dispose statement at the
top of that method body instead of stacking a fresh 'this.Closed += (s,e) => ...'
lambda. Cleaner, more idiomatic patches.
Conservative: only folds into a clean BLOCK-body OnClosed (its '{' ends a line);
one-liner/expression bodies fall back to the lambda. Other owners (Unloaded /
non-Window) keep the lambda. detail reported as '<ev>/fold'.
Fixture: FoldWindow (Window with an existing OnClosed). +1 test. 21/21 + 7/7, -O.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoaAdd two more unambiguous INotify-family event delegates to the lambda-extraction table (PropertyChangingEventArgs, DataErrorsChangedEventArgs). Names whose delegate differs across frameworks (Click, TextChanged -> RoutedEventArgs vs EventArgs) are deliberately NOT added — extracting them blindly could emit a wrong signature, so they stay suggest-only. +1 test. 22/22 + 7/7, -O. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe OWN fixer now folds teardown statements into an existing ChangesOWN fixer teardown and extraction updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:1747ac37a5
ℹ️ 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".
| fold = _fold_target(lines, cls_idx, ev) if cls_idx is not None else None | ||
| if fold is not None: | ||
| brace_idx, body_indent = fold | ||
| return [(brace_idx + 1, brace_idx + 1, [f"{body_indent}{stmt};\n"])], f"{ev}/fold" |
There was a problem hiding this comment.
When the subscribed source is a constructor parameter or local, the old fallback inserted a Closed lambda next to the subscription and captured that variable. This fold emits the raw detach statement inside OnClosed; for a finding like goods.PropertyChanged += GoodsPropertyChanged in a constructor, the generated OnClosed contains goods.PropertyChanged -= ..., where goods is out of scope, so the fixer produces an uncompilable patch instead of a reviewable fix. Only fold sources proven to be class members/in scope, otherwise keep the lambda path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex Fixed in d840115. Folding now only happens when the subscription source is a class member (in scope inside OnClosed). _is_class_member checks the root of src.Event — members carry an access/field modifier, ctor params and locals don't, so they're distinguished reliably. A ctor-local source falls back to the capturing lambda at the call site (where it's in scope). Test: test_fold_skips_ctor_local_source.
Generated by Claude Code
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
| # so extracting them blindly could emit a wrong signature → they stay suggest-only. | ||
| _EVENT_ARGS = { | ||
| "PropertyChanged": "PropertyChangedEventArgs", | ||
| "PropertyChanging": "PropertyChangingEventArgs", |
There was a problem hiding this comment.
These newly enabled events can be subscribed with lambdas in files that do not import System.ComponentModel; the generated method then uses PropertyChangingEventArgs/DataErrorsChangedEventArgs unqualified, while the original lambda did not require those type names. In that case the fixer turns a previously skipped finding into a patch that fails to compile with a missing-type error, so qualify these types or only apply the extraction when the namespace is already in scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex Fixed in d840115. The _EVENT_ARGS types are now fully qualified (System.ComponentModel.PropertyChangingEventArgs, System.Collections.Specialized.NotifyCollectionChangedEventArgs, etc.), so the extracted method compiles regardless of the file's usings — the original lambda relied on inference, the named method can't. Tests updated to assert the qualified signatures.
Generated by Claude Code
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
fix/tests/test_own_fix.py (2)
178-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlease cover
ErrorsChangedin this delegate test.
_EVENT_ARGSgained two new mappings in this PR, but this test only exercisesPropertyChanging. AnErrorsChangedcase would catch regressions in the newDataErrorsChangedEventArgspath.🤖 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 `@fix/tests/test_own_fix.py` around lines 178 - 199, Add an `ErrorsChanged` scenario to `test_lambda_extraction_more_delegates` so the delegate extraction path is validated for the new `DataErrorsChangedEventArgs` mapping as well. Extend the test setup around `plan_file` and the `Finding`/`applied` assertions to use an `m.ErrorsChanged += ...` subscription, then verify the generated handler signature uses `DataErrorsChangedEventArgs` and still applies `extract+detach` like the existing `PropertyChanging` case.
118-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the new
Closed/foldoutcome here too.This proves the C# was folded, but the PR also changes the applied-detail contract to
Closed/fold. Right now_applieris ignored, so a regression back to plainClosedwould still pass.🤖 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 `@fix/tests/test_own_fix.py` around lines 118 - 128, The test currently only verifies the folded C# content and ignores the applied-detail contract, so it would still pass if the result regressed from Closed/fold to plain Closed. Update test_own001_subscription_folds_into_onclosed to assert the _applier-provided outcome includes Closed/fold, alongside the existing OnClosed and lambda checks, so the contract change is covered.
🤖 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 `@fix/fixarm/own_fix.py`:
- Around line 237-249: Restrict _fold_target() so it only considers an OnClosed
override in the owning class body, not nested types. Update the scan in
_fold_target (and any helper it uses such as
_class_close/_fold_after_open_brace) to track brace depth and accept a match
only at member depth within the current class, preventing outer classes from
folding into a nested type’s OnClosed and inserting cleanup into the wrong C#
type.
---
Nitpick comments:
In `@fix/tests/test_own_fix.py`:
- Around line 178-199: Add an `ErrorsChanged` scenario to
`test_lambda_extraction_more_delegates` so the delegate extraction path is
validated for the new `DataErrorsChangedEventArgs` mapping as well. Extend the
test setup around `plan_file` and the `Finding`/`applied` assertions to use an
`m.ErrorsChanged += ...` subscription, then verify the generated handler
signature uses `DataErrorsChangedEventArgs` and still applies `extract+detach`
like the existing `PropertyChanging` case.
- Around line 118-128: The test currently only verifies the folded C# content
and ignores the applied-detail contract, so it would still pass if the result
regressed from Closed/fold to plain Closed. Update
test_own001_subscription_folds_into_onclosed to assert the _applier-provided
outcome includes Closed/fold, alongside the existing OnClosed and lambda checks,
so the contract change is covered.
🪄 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 Plus
Run ID: 228d6a58-8ddc-42f7-8d52-42d8753c6be5
📒 Files selected for processing (6)
fix/README.mdfix/fixarm/own_fix.pyfix/fixtures/own001-sub-fold/after.findings.jsonfix/fixtures/own001-sub-fold/before.findings.jsonfix/fixtures/own001-sub-fold/before/Broker/FoldWindow.xaml.csfix/tests/test_own_fix.py
Uh oh!
There was an error while loading. Please reload this page.
- fold only when the source is a class MEMBER (Codex P2): folding a raw detach into OnClosed is out of scope if the source is a ctor param/local. _is_class_member gates it (members carry a modifier; params/locals don't); non-members keep the capturing lambda at the call site. (+test: ctor-local source -> lambda) - fully-qualify extracted event-args types (Codex P2): the generated method names the type explicitly, so PropertyChangingEventArgs etc. are now System.ComponentModel.* — compiles even if the file lacked the using. - restrict OnClosed fold to member depth (CodeRabbit Major): depth-gated scan so a nested type's OnClosed can't be folded into. (+test: nested OnClosed -> lambda) - tests: assert the 'Closed/fold' applied-detail; cover ErrorsChanged extraction. 24/24 own + 7/7 wrapper, normal and -O. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fix/tests/test_own_fix.py (1)
236-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisplaced section divider.
The
# ---- refused shapes stay suggest-only: NOT patched ---divider sits directly abovetest_lambda_extraction_more_delegates, which asserts the opposite — that these shapes ARE patched (extract+detach). The divider appears to belong to the refused-shape tests further down; move it below this test to avoid misreading intent. Test logic itself is correct.🤖 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 `@fix/tests/test_own_fix.py` around lines 236 - 264, The section divider comment is misplaced above test_lambda_extraction_more_delegates, where the assertions expect extract+detach rather than suggest-only. Move the divider so it sits with the refused-shape tests that actually remain unpatched, and keep test_lambda_extraction_more_delegates grouped with the patched event extraction cases. Use the test function name test_lambda_extraction_more_delegates and the divider text “refused shapes stay suggest-only” to locate the block.
🤖 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 `@fix/fixarm/own_fix.py`:
- Around line 248-250: The OnClosed match in the fold-detach logic is still
checking the raw source line, so commented-out or string text can trigger a
false fold anchor. Update the detection in the loop around the OnClosed handling
to run the regex against _code_skeleton(lines[i]) instead of lines[i], keeping
it consistent with the depth-based gating and ensuring _fold_after_open_brace
only fires on real declarations.
---
Nitpick comments:
In `@fix/tests/test_own_fix.py`:
- Around line 236-264: The section divider comment is misplaced above
test_lambda_extraction_more_delegates, where the assertions expect
extract+detach rather than suggest-only. Move the divider so it sits with the
refused-shape tests that actually remain unpatched, and keep
test_lambda_extraction_more_delegates grouped with the patched event extraction
cases. Use the test function name test_lambda_extraction_more_delegates and the
divider text “refused shapes stay suggest-only” to locate the block.
🪄 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 Plus
Run ID: 0f3eef6a-c10c-4a45-aaeb-02a5fe83d6de
📒 Files selected for processing (2)
fix/fixarm/own_fix.pyfix/tests/test_own_fix.py
Uh oh!
There was an error while loading. Please reload this page.
- _fold_target now matches the OnClosed override against _code_skeleton(line) instead of the raw line, so a commented-out/string '//... OnClosed ...' at member depth can't be picked as a bogus fold anchor. (+test) - move the 'refused shapes' test divider below the extraction test it was mistakenly heading (cosmetic). 25/25 own + 7/7 wrapper, normal and -O. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
Uh oh!
There was an error while loading. Please reload this page.
OWN fixer: fold into
OnClosed+ wider lambda delegatesTwo quality follow-ups on the OWN001 fixer (both Linux/CI-native, conservative).
1. Fold cleanup into an existing
OnClosedoverrideWhen the owner class already has a
protected override void OnClosed(...)— which runs exactly when the Window'sClosedevent would — the detach/dispose statement is inserted at the top of that method body instead of stacking anotherthis.Closed += (s,e) => …lambda:protected override void OnClosed(EventArgs e) { + fGoods.PropertyChanged -= new PropertyChangedEventHandler(GoodsPropertyChanged); base.OnClosed(e); }Conservative: only folds into a clean block-body
OnClosed(its{ends a line); one-liner/expression bodies and non-Window owners fall back to the lambda. Reported as<ev>/fold.2. Wider lambda-extraction delegates
Added two more unambiguous INotify-family event delegates:
PropertyChanging→PropertyChangingEventArgs,ErrorsChanged→DataErrorsChangedEventArgs. Names whose delegate differs across frameworks (Click,TextChanged→RoutedEventArgsvsEventArgs) are deliberately not added — extracting them blindly could emit a wrong signature, so they stay suggest-only.Test plan
Fixtures:
FoldWindow(existingOnClosed) + a syntheticPropertyChangingextraction.Not in this PR (Windows-bound)
Promoting analyzer rules to
tiers._T1_RULESneeds realroslynatordiffs to judge safely, and theroslynator fix Broker.slnspike needs MSBuild — both wait on the Windows stand (docs/fix-arm.md§6).🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Closedsubscription cleanup can now fold detach logic directly into an existingprotected override OnClosed(...), avoiding addedthis.Closed += …handlers.OnClosedonly on nested types).ErrorsChangedhandling.Documentation
Tests