Fix api collection icon rendering

[RalucaP]

Bug/issue: rdar://135837611

Summary

This PR addresses an issue where API Collections were displaying incorrect icons in external references. The issue occurred when external references to API Collections were rendering with "role": "article" instead of "role": "collectionGroup", causing them to display article icons instead of the proper collection icons.

The implementation adds the missing .collectionGroup case to the renderKindAndRole function to ensure API Collections render consistently as articles with the collectionGroup role, displaying the correct collection icon in both main documents and external references.

Dependencies

None.

Testing

The functionality can be tested using the provided test case in DocumentationContentRendererTests.swift:

func testRenderKindAndRoleForAPICollection() throws {
    // API Collections should render as articles with collectionGroup role to display correct icon
    let (collectionKind, collectionRole) = DocumentationContentRenderer.renderKindAndRole(.collectionGroup, semantic: nil)
    XCTAssertEqual(collectionRole, "collectionGroup", "API Collections should have collectionGroup role for correct icon")
    
    // Verify other node types still work correctly
    let (articleKind, articleRole) = DocumentationContentRenderer.renderKindAndRole(.article, semantic: nil)
    XCTAssertEqual(articleKind, RenderNode.Kind.article)
    XCTAssertEqual(articleRole, "article")
    
    let (symbolKind, symbolRole) = DocumentationContentRenderer.renderKindAndRole(.class, semantic: nil)
    XCTAssertEqual(symbolKind, RenderNode.Kind.symbol)
    XCTAssertEqual(symbolRole, "symbol")
}

Checklist

• Updated tests
• Ran the ./bin/test script and it succeeded
• Updated documentation if necessary

[patshaughnessy]

This change looks good.

There are many different DocumentationNode.Kind values... have we tested if any other icons are incorrect?

[RalucaP]

Hi Pat, I made some changes to address the radar concerns for cross-framework API collection icons, can you please take another look?

There are many different DocumentationNode.Kind values... have we tested if any other icons are incorrect?

This radar is specific for API Collections, if more icons appear incorrect, we should fix them too, but probably in different PRs. :) Thanks!

[patshaughnessy]

Some minor suggestions. Thanks for the fix!

[d-ronnqvist]

We can't rely on the task groups information for this. Is there some other information that indicates that a external page is an API collection?

[d-ronnqvist]

Question: Is there some other value about the link summary that we can use to determine if it's an API collection?

[RalucaP]

After analyzing the LinkDestinationSummary structure and the actual Apple documentation data, I found that there are no reliable alternatives to taskGroups for identifying API Collections in external references. The available fields (title patterns, URL patterns, abstract content, references) would all be fragile heuristics.
We could add an optional role to LinkDestinationSummary, but that would be a much larger change. Would you prefer that?

[d-ronnqvist]

The taskGroup information isn't a reliable source for this information. It's not requited for documentation sources to include it and we are also talking about removing it because the link summary isn't meant to be a representation of the documentation hierarchy.

[d-ronnqvist]

Is there another DocumentationNode.Kind value that API collection pages should be using? I see that we have both collection and collectionGroup but I don't recall what either of those are meant to represent. If not, should we introduce a dedicated "API Collection" kind and use that?

[patshaughnessy]

I believe collection is the root/framework page, and collectionGroup are API Collections or pages that groups symbol pages together.

[d-ronnqvist]

Reading the kind values documentation I would think that a framework would use module. Regardless, if we have a kind value that represents API collections then it sounds like we should use that to identify API collections.

[RalucaP]

Thank you, both! I spoke with @d-ronnqvist and ended up taking a different approach, let me know your thoughts. :)

[d-ronnqvist]

This looks like a good solution to me.

I'm curious how the code would behave if the API Collection specified an explicit "Article" kind using

@Metadata {
  @PageKind(article)
}

I feel that an explicit page kind like that should override any default kind inferred from the page's content. I think that maybe the new code already works like this, but it would be good to have a second test that verifies that behavior.

(none of the individual questions or minor feedback are blocking)

[d-ronnqvist]

minor: We have a test helper that does this

Suggested change

	// Verify round-trip encoding preserves the correct kind
	let encoded = try JSONEncoder().encode(pageSummary)
	let decoded = try JSONDecoder().decode(LinkDestinationSummary.self, from: encoded)
	XCTAssertEqual(decoded.kind, .collectionGroup)
	try assertRoundTripCoding(pageSummary)

[d-ronnqvist]

minor: If this page didn't produce any summaries, then line would trap and stop running all the other tests. If you use an optional expression that's wrapped in XCTUnwrap, then if the code would behave unexpectedly, the failure would be limited to this test, and the rest would continue to run.

Suggested change

	let pageSummary = summaries[0]
	let pageSummary = try XCTUnwrap(summaries.first)

[d-ronnqvist]

minor: This title is oddly specific for a test

Suggested change

	# Time Pitch Algorithm Settings
	# Some API collection

[RalucaP]

Updated! Changed from specific 'Time Pitch Algorithm Settings' to generic 'API Collection' title to avoid confusion with real-world examples.

[d-ronnqvist]

minor: This topic name doesn't match the symbol that it's curating.

Because the API collection only has a single topic section, you can probably remove its title if we don't have a better title for it.

Suggested change

### Algorithms

[d-ronnqvist]

Question: is it intentional that the Info.plist file specifies a name that's different from the module name? That's a bit unusual (but completely valid).

I'm asking because catalogs generally don't need an Info.plist file except that in this test you're hardcoding its name below when creating the reference, so removing it would cause the API collection to not be found for that reference string.

[RalucaP]

I believe this is correct! 'TestBundle' (display name) vs 'TestModule' (module name) should follow the convention for testing cross-module references.

[d-ronnqvist]

This test doesn't verify any cross-module linking behaviors and even then it's uncommon and a bit unconventional to specify a display name in the Info.plist that's different from the module name. It's more common for documentation to either explicitly specify the same display name as the module name (often redundantly) or to not have an Info.plist at all.

[RalucaP]

Done! Removed the Info.plist from both test methods since they focus on API Collection kind detection rather than cross-module linking. Thanks!

[RalucaP]

Thanks for the thorough review, @d-ronnqvist ! All feedback addressed with improved test quality, better error handling, and added regression test for @PageKind precedence. :)
The fix maintains backward compatibility while correctly identifying API Collections in external references.

I'm curious how the code would behave if the API Collection specified an explicit "Article" kind using

@Metadata {
  @PageKind(article)
}

Good catch! Added testExplicitPageKindOverridesAPICollectionDetection() to verify explicit @PageKind(article) takes precedence over automatic detection. The existing precedence logic already handles this correctly.

[d-ronnqvist]

@swift-ci please test