fix: remove trailing / from site-url and ensure canonical gets one

[mcanouil]

This pull request fixes the issue where a trailing slash was not being removed from the site URL. The synthesizeCitationUrl function now removes the trailing slash from the base URL before constructing the citation URL.

Outdated

I did not change the "else" part because I was unsure how to trigger this part of the codepath, but using the same if/else condition on length would not hurt I believe.

    } else {
      const relativePath = relative(
        rootDir,
        join(dirname(input), basename(outputFile)),
      );
      const part = pathWithForwardSlashes(relativePath);
+     if (part.length === 0) {
-     return `${baseUrl}/${part}`;
+       return `${baseUrl}/${part}`;
+     } else {
+       return `${baseUrl}/${part}/`;
+     }
    }

Fixes #9524

[cscheid]

(Note the failing tests.)

[cscheid]

This suggestion doesn't work.

Use the following as a test:

_quarto.yml

project:
  type: website

canonical-url: true

website:
  site-url: https://index.html.com/
  title: "Test-9595"
  navbar:
    left:
      - href: index.qmd
        text: Home
      - about.qmd

format:
  html:
    theme: cosmo
    css: styles.css
    toc: true

You'll see that about.html gets a broken canonical URL:

<link rel="canonical" href="https://index.html.com/about.html/">

[mcanouil]

Weird, as I don't get this result using this current PR with the two additional commits.

FYI, I am using https://github.com/mcanouil/quarto-issues-experiments/tree/main/quarto-cli-9524

[mcanouil]

I've still got an issue though but not for index related files.

[cscheid]

The later commits fixed it.

[mcanouil]

Yep, I did not realised, the else part always returned the file, so trailing / was not needed.
My reprex has no issues and tests were fine locally.

Waiting for CI to confirm.

[cscheid]

I'd like some unit or smoke tests here.

Can you create a website project under 2024/05/06, configure it like above, and then test the contents of index.qmd and about.qmd? You can use tests/docs/smoke-all/2024/04/17/9356 as a template. (I think you'll need to use ensureFileRegexMatches instead of ensureHtmlElements, though)

[mcanouil]

I'll add the test probably on wednesday (I was going through the smoke tests to find a "template").

[cscheid]

(I think you'll need to use ensureFileRegexMatches instead of ensureHtmlElements, though)

[mcanouil]

I don't understand why the tests are failing for subdirectories but not for root documents.
The resulting HTML documents do contain the values but somehow the same regex test does not behave the same way.
I am very confused here.

[cderv]

I don't understand why the tests are failing for subdirectories but not for root documents.

I think this is just because our smoke all testing don't support this organization for projects. Your current test project is not something that smoke-all.tests.ts support. So it requires to improve and add support.

See the path in error log

Inspecting docs/smoke-all/2024/05/06/9524/about/test/_site/something.html for Regex matches

• Look at where you project lives (docs/smoke-all/2024/05/06/9524/)
• Where the subdirectory file is supposed to be (in about/test)
• What is the project output dir (_site)

You see that the path created as output for the file to test is wrong, so you get the No such file or directory error.

This is because we don't support subdirectory in project we tests in smoke-all

quarto-cli/tests/utils.ts

Lines 89 to 94 in 3b4cdd3

	const outputPath = projectOutDir
	? join(dir, projectOutDir, `${stem}.${outputExt}`)
	: join(dir, `${stem}.${outputExt}`);
	const supportPath = projectOutDir
	? join(dir, projectOutDir, `${stem}_files`)
	: join(dir, `${stem}_files`);

We build the path without accounting for possibly subdirectories for the input.

So two solutions:

• Don't do smoke-tests but regular ts test, where you get more control on which file to tests
• Make adaptation to outputForInput() and possibly other places to fix this project testing with smoke-all

Hope it helps understand

[cscheid]

@mcanouil Responding to #9524 (comment) here since it's more relevant:

The test suite still isn't passing, right?

[mcanouil]

Yes, because website project subdirectories does not meet test setup workflow as explained by Christophe.
So the smoke test does not seem suited to test this which would need to test in a real project.

(Also did not had time to look in other testing workflows since I came back from my holidays)

Edit: Manual inspection shows proper URLs. Root files are successfully passing the tests. The same tests for subdirectories fail.

[cderv]

Let me try to adapt the test here.

[cderv]

So I have done the work in an other PR:

• Adapt our test logic to correctly support project with subfolders #9969

When this is merged, the test here should pass, possibly without change as it will now behave as expected.

[cscheid]

Should just be a matter of merging this against main in the PR branch, then, so the test suite runs with the new changes?

[cderv]

@mcanouil All test have passed now. So this should be good.

PR is draft - did you get anything to do before we merge ?

[mcanouil]

I put it in draft because of the failing tests, so ready it is since the changes were made in main.

[cderv]

I'll rebase and merge. Thanks !