fix: tree generation for autogenerated groups

[kevinzunigacuellar]

Description

• Closes support for directory index pages #370
• This PR modifies treeify() to build the file tree depth first. This way if a file and a directory in within the same level have the same name, the file is included into the directory and renamed to index.

Before

reference
 |- foo.md
 |- foo
     |- bar.md

After

reference
 |- foo
     |- bar.md
     |- index.md

[changeset-bot]

🦋 Changeset detected

Latest commit: edc95e1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@astrojs/starlight	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
starlight	✅ Ready (Inspect)	Visit Preview	Jan 18, 2024 5:26pm

1 Ignored Deployment

Name	Status	Preview	Updated (UTC)
starlight-i18n	⬜️ Ignored (Inspect)	Visit Preview	Jan 18, 2024 5:26pm

[astrobot-houston]

size-limit report 📦

Path	Size
/index.html	5.21 KB (0%)
/_astro/*.js	21.42 KB (0%)
/_astro/*.css	13.15 KB (0%)

[HiDeoo]

I did not review the code in details yet altho I noticed a few sorting details that look weird to me when playing with the changes locally. The documentation says:

Autogenerated sidebar groups are sorted by filename alphabetically. For example, a page generated from astro.md would appear above the page for starlight.md.

Considering the existing file structure for the reference docs:

- src/
  - content/
    - docs/
      - reference/
        - configuration.md
        - frontmatter.md
        - overrides.md
        - plugins.md

I get the following sidebar for the reference folder:

- Configuration Reference
- Frontmatter Reference
- Overrides Reference
- Plugins Reference

If I add a docs/src/content/docs/reference/frontmatter/a.md file, I get the following sidebar:

- frontmatter
  - Frontmatter Reference
  - A test
- Configuration Reference
- Overrides Reference
- Plugins Reference

The 2 things that look weird to me are (and I'm not sure if they are related):

• In the frontmatter folder, I think A test should be above Frontmatter Reference (because of the alphabetical order mentioned in the docs)
• In the root reference folder, I think frontmatter should be below Configuration Reference right?

[kevinzunigacuellar]

Thank you for the kind review @HiDeoo! While examining the implementation, it seems that this bug is not related to the tree generation, however it is currently in production.

starlight/packages/starlight/utils/navigation.ts

Line 227 in ea917d8

return collator.compare(isDir(a) ? keyA : a.slug, isDir(b) ? keyB : b.slug);

I think we meant to sort using the data.title of the file instead of the slug. That's why frontmatter is at the top because it is compared to reference/configuration instead of just Frontmatter Reference. I will follow up with another PR.

This bug also explains why A test is not at the top of frontmatter.

[HiDeoo]

I think we meant to sort using the data.title of the file instead of the slug.

I'm not sure about this one, as the docs says:

Autogenerated sidebar groups are sorted by filename alphabetically.

So it should clearly be the file or directory name and not the title, right?

I based my two observations on this as:

• A test is the title of the file a.md so should be before frontmatter.md (I could have use Zzzzz for the title to make it more obvious)
• A frontmatter directory should be after a file named configuration.md

[delucis]

Oops! Looks like stuff broke when I updated branch. Can you take a look @kevinzunigacuellar?

[kevinzunigacuellar]

Fixed! it seem to be missing an import for basename. Also I had to update the test snapshot. It is correctly sorted now.

[delucis]

Thanks again for getting into this gnarly logic @kevinzunigacuellar! Left a few comments.

[delucis]

Should this item show first like it would if created via reference/frontmatter/index.md?

Not super simple to solve at first glance: our getComparisonId() helper added in #1298 doesn’t know if a file is a directory index, so reference/frontmatter.md is not treated the same as reference/frontmatter/index.md (the first returns 'frontmatter', the latter '').

I’m indecisive on whether it’s better to use an extra property on Route, e.g. sortId or something that we add, so a route could look like { id: 'reference/frontmatter.md', sortId: 'reference/frontmatter/index.md' }, OR whether our sorting logic should use a heuristic when comparing, e.g. when comparing 'reference/frontmatter.md' and 'reference/frontmatter/foo.md', turn the former into '' because it matches the directory (e.g. if stripExtension(idA) === dirname(idB), idA is an index).

Setting a sortId early would avoid those extra checks, so might be cleanest, even if it does add another property (we’d now have id, sortId, and entry.id). Doing it when sorting (I just tried 😁) is pretty gross.

[kevinzunigacuellar]

good catch! Perhaps this sorting id could be generated when creating the tree. Everytime we go into a tree node recursively we could attach the current object property as a path. This way not only files but directories can also have a sorting id.

[delucis]

Do directories need them? It’s only for this one case I think. But yeah, I think doing it then makes sense!

[kevinzunigacuellar]

I think it could make a more homogeneous comparison. Since it was not possible to know the directory path, comparisons between directories and files were done by comparing a filename and directory name. But if both were in include their paths it would make a bit easier.

[kevinzunigacuellar]

On second thought I think It would be a better solution to include a slug property in directories dir["_slug"] since index pages defined as index or pages of the parent directory share the same slug.

Route	slug
reference/frontmatter.md	reference/frontmatter
reference/frontmatter/index.md	reference/frontmatter

Then sorting by slug would be very trivial for both directories and files

Order	Route/Dir	slug (use this to sort)
1	reference/frontmatter/index.md	reference/frontmatter
2	reference/frontmatter/bar	reference/frontmatter/bar
3	reference/frontmatter/bar/foo.md	reference/frontmatter/bar/foo
4	reference/frontmatter/fiz	reference/frontmatter/fiz
5	reference/frontmatter/fiz/zoo.md	reference/frontmatter/fiz/zoo
6	reference/frontmatter/foo.md	reference/frontmatter/foo

Sidebar view

 reference/
    ├── index.md
    ├── bar/
    │   └── foo.md
    ├── fiz/
    │   └── zoo.md
    └── foo.md

This way index are always at the top, even in nested directories.

[delucis]

Hmm, maybe? Not opposed to trying!

[kevinzunigacuellar]

added in f4ee4ed, I need some help with types but everything seem to be working

[delucis]

@kevinzunigacuellar I pushed a fix for the type issue in dc2292a!

To describe what was happening: the _slug property you tried adding collided with the generic [item: string]: Dir | Route. Trying to set an additional property with a string index was an error, because all string-indexable properties are declared to be Dir or Route, not string.

Instead, I followed the same pattern as we do for the DirKey: create a Symbol to index with, which we can then use to look up the value.

Also took the liberty of fleshing out the changeset and bumping this up to a minor just in case it has some unexpected side effect (I don’t think it should though).

Should be good to go! I will merge when we’re ready to ship the next minor. Thanks again for the dedication figuring out all the tricky corners of this one 💖

[kevinzunigacuellar]

Nice refactor, thanks for polishing the final details of the implementation.

Self LGTM! 😆