Skip to content

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Sep 17, 2025

Fixes a bug where the same sourcemap can be associated with mulitple source files.

Change description

We solve this problem by keeping track of the sourcemaps we have already associated with a source file. If we guess a sourcemap which has already been associated, we do not associate the given source file with any sourcemap.

Also, we iterate first through all the sources which explicitly reference the sourcemap – i.e., for which, we don't have to guess the sourcemap. These are associated first. As they are explicitly linked to a sourcemap, we intentionally do not prevent duplicates here.

On the second iteration, we guess the sourcemap reference for all remaining sources. We enforce the no duplicates rule on the second iteration. Sources cannot be associated with a sourcemap that was explicitly referenced by another source on the first iteration. If multiple sources guess the same sourcemap, and that sourcemap is not explicitly associated with any source, then none of the sources will be associated with that sourcemap (as it is unclear how that sourcemap should be associated).

In all cases, we warn the users that we failed to associate the sources properly.

Issues

Fixes #2445
Fixes CLI-5

@szokeasaurusrex szokeasaurusrex changed the title Szokeasaurusrex/bug-double-inject WIP: Fix double inject bug Sep 17, 2025
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/bug-double-inject branch from a1ca2bd to 7e16121 Compare September 18, 2025 17:09
@szokeasaurusrex szokeasaurusrex changed the title WIP: Fix double inject bug fix(sourcemaps): Fix double association bug Sep 18, 2025
Copy link

linear bot commented Sep 18, 2025

@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review September 18, 2025 17:17
@szokeasaurusrex szokeasaurusrex requested a review from a team as a code owner September 18, 2025 17:17
cursor[bot]

This comment was marked as outdated.

@szokeasaurusrex szokeasaurusrex marked this pull request as draft September 18, 2025 17:26
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/bug-double-inject branch from 7e16121 to 6fc0961 Compare September 26, 2025 13:19
@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review September 26, 2025 16:10
cursor[bot]

This comment was marked as outdated.

@lcian lcian self-assigned this Sep 29, 2025
Copy link
Member

@lcian lcian left a comment

Choose a reason for hiding this comment

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

The high level idea and implementation look good to me after carefully reading through the code.

I would suggest simplifying the code to prioritize ease of understanding. It's easy to lose track when there's a lot of nesting like this.

Maybe some of the processing logic can be extracted into functions.
If this is not possible/beneficial, we could at least add comments to the for_each/fold/filter steps where the body is multiple lines, to make it easier to reason about specific steps in isolation.

@szokeasaurusrex szokeasaurusrex marked this pull request as draft September 30, 2025 11:10
@szokeasaurusrex
Copy link
Member Author

Converting to draft while I work on the simplification, per @lcian's request

@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review October 3, 2025 13:58
@szokeasaurusrex
Copy link
Member Author

@lcian is this good now?

@szokeasaurusrex szokeasaurusrex requested a review from lcian October 3, 2025 13:58
Copy link
Member

@lcian lcian left a comment

Choose a reason for hiding this comment

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

Looks good and much easier to understand, thanks!

@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) October 9, 2025 15:30
@szokeasaurusrex szokeasaurusrex merged commit a38ecaa into master Oct 9, 2025
25 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/bug-double-inject branch October 9, 2025 15:34
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.

Ensure collect_sourcemap_references does not associate the same sourcemap with multiple source files
2 participants