Skip to content

Conversation

@cotti
Copy link
Contributor

@cotti cotti commented Nov 11, 2025

Fixes #2190

On assembler builds, image URLs were not being adjusted to their final paths. We can access this information through the context's PositionalNavigation.

@cotti cotti self-assigned this Nov 11, 2025
@cotti cotti requested a review from a team as a code owner November 11, 2025 23:25
@cotti cotti requested a review from reakaleek November 11, 2025 23:25
@cotti cotti added the fix label Nov 11, 2025
@cotti cotti enabled auto-merge (squash) November 11, 2025 23:54
Copy link
Member

@reakaleek reakaleek left a comment

Choose a reason for hiding this comment

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

Would testing help verify this?

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

I think we can simplify this a bit using:

		if (context.Build.AssemblerBuild && context.TryFindDocument(fi) is MarkdownFile currentMarkdown)
		{
			// Acquire navigation-aware path
			if (context.PositionalNavigation.MarkdownNavigationLookup.TryGetValue(currentMarkdown, out var currentNavigation))
			{
				var uri = new Uri(new UriBuilder("http", "localhost", 80, currentNavigation.Url).Uri, url);
				newUrl = uri.AbsolutePath;
			}
			else
				context.EmitError($"Failed to acquire navigation for current markdown file '{currentMarkdown.FileName}' while resolving relative url '{url}'.");
		}

@reakaleek
Copy link
Member

Would testing help verify this?

I took a stab at creating a appropriate test for this:

#2199

I was able to verify, that the test succeeds with your changes and fails without your changes.

@cotti cotti requested review from Mpdreamz and reakaleek November 13, 2025 14:03
Copy link
Member

@reakaleek reakaleek left a comment

Choose a reason for hiding this comment

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

What are your thoughts on the changes suggest by CodeQL?

@Mpdreamz
Copy link
Member

A few of those codeql suggestions might not be relevant if we use the Uri approach I suggested

@cotti
Copy link
Contributor Author

cotti commented Nov 13, 2025

Let's get them in, then!

cotti and others added 2 commits November 13, 2025 11:10
…solving (Copilot)

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
@reakaleek
Copy link
Member

Let's get them in, then!

I think @Mpdreamz meant that with the Uri approach, you don't need the custom url combine method.

@cotti
Copy link
Contributor Author

cotti commented Nov 13, 2025

Ah. Adjusting here.

@cotti cotti requested a review from reakaleek November 13, 2025 15:35
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
@cotti cotti merged commit 0df0d25 into main Nov 13, 2025
24 checks passed
@cotti cotti deleted the fix/image_paths branch November 13, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Images from reference repos are not loading

4 participants