Skip to content

Cache remark ownership lookup - #331

Merged
HassanAkbar merged 2 commits into
fix/function-body-tail-remarksfrom
fix/pr-330-schema-validation-performance
Aug 6, 2026
Merged

Cache remark ownership lookup#331
HassanAkbar merged 2 commits into
fix/function-body-tail-remarksfrom
fix/pr-330-schema-validation-performance

Conversation

@HassanAkbar

Copy link
Copy Markdown
Member

related to #330

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Improves performance and correctness of remark attachment by memoizing the node ownership lookup used when selecting the innermost candidate node for body comments, and ensuring memoized state is cleared after attachment (including on exceptions).

Changes:

  • Memoize the node ownership map (owner_map) and reuse it across repeated innermost_candidate calls during a single attachment run.
  • Update innermost_candidate to rely on the memoized ownership map rather than rebuilding from the passed node list.
  • Add specs verifying the ownership map is built once and is cleared (along with the node index) if attach raises.

Reviewed changes

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

File Description
spec/expressir/express/remark_attacher_spec.rb Adds coverage for ownership-map memoization and cleanup-on-exception behavior.
lib/expressir/express/remark_attacher.rb Introduces memoized owner_map, updates innermost_candidate to use it, and clears it in attach’s ensure block.

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

@HassanAkbar
HassanAkbar marked this pull request as ready for review August 6, 2026 12:42
@HassanAkbar
HassanAkbar merged commit ffbb744 into fix/function-body-tail-remarks Aug 6, 2026
15 checks passed
@HassanAkbar
HassanAkbar deleted the fix/pr-330-schema-validation-performance branch August 6, 2026 12:43
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.

2 participants