Skip to content

Conversation

@RalucaP
Copy link
Contributor

@RalucaP RalucaP commented Nov 10, 2025

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

Copy link
Contributor

@patshaughnessy patshaughnessy left a comment

Choose a reason for hiding this comment

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

This change looks good.

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

Adds test to verify that articles with task groups from external references get collectionGroup role for correct icon display. The test is temporarily skipped until the fix is implemented in the next commit.

rdar://135837611
Implements detection of API Collections in external references by checking for task groups in LinkDestinationSummary. Articles with task groups are treated as API Collections and get collectionGroup role for correct icon display. Removes test skip to enable validation of the fix.

rdar://135837611
@RalucaP RalucaP force-pushed the fix-api-collection-icon-rendering branch from 9213698 to b385a24 Compare November 12, 2025 15:37
@RalucaP
Copy link
Contributor Author

RalucaP commented Nov 12, 2025

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!

Copy link
Contributor

@patshaughnessy patshaughnessy left a comment

Choose a reason for hiding this comment

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

Some minor suggestions. Thanks for the fix!

/// Create a topic render render reference for this link summary and its content variants.
func makeTopicRenderReference() -> TopicRenderReference {
let (kind, role) = DocumentationContentRenderer.renderKindAndRole(kind, semantic: nil)
let (kind, role) = DocumentationContentRenderer.renderKindAndRole(kind, semantic: nil, linkSummary: self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let (kind, role) = DocumentationContentRenderer.renderKindAndRole(kind, semantic: nil, linkSummary: self)
let (kind, role) = DocumentationContentRenderer.renderKindAndRole(kind, linkSummary: self)

You set the default value for semantic to be nil, so there's no need to pass that in here now.

)
}

func testRenderKindAndRoleForAPICollection() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

Test looks good, but you might want to add another test for the case when taskGroups is nil, i.e. test that this continues to work for references to a normal article.

And what should happen if taskGroups is an empty array?

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

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

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?

default:
if let article = semantic as? Article {
return (.article, roleForArticle(article, nodeKind: kind).rawValue)
} else if kind == .article, let summary = linkSummary, summary.taskGroups != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The task groups of an link summary isn't a value that external documentation sources have to provide—and DocC doesn't include it in its link summaries unless specifically configured to do so—so it can't be reliably used to determine if an external page is an API collection or not.

/// Create a topic render render reference for this link summary and its content variants.
func makeTopicRenderReference() -> TopicRenderReference {
let (kind, role) = DocumentationContentRenderer.renderKindAndRole(kind, semantic: nil)
let (kind, role) = DocumentationContentRenderer.renderKindAndRole(kind, semantic: nil, linkSummary: self)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

static func renderKindAndRole(_ kind: DocumentationNode.Kind?, semantic: Semantic?) -> (RenderNode.Kind, String) {
static func renderKindAndRole(_ kind: DocumentationNode.Kind?, semantic: Semantic? = nil, linkSummary: LinkDestinationSummary? = nil) -> (RenderNode.Kind, String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand this correctly, this method is not expected to receive both a semantic and a link summary because one represents a local page and the other represents an external page. If we need a version that accepts a link summary we could consider having either a separate overload (that calls into a private common implementation) or a parameter that can be either a Semantic or a LinkDestinationSummary.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not expected to pass both semantic and linkSummary at the same invocation call, let's split the method into two making them an overload.

The original

 static func renderKindAndRole(_ kind: DocumentationNode.Kind?, semantic: Semantic?) -> (RenderNode.Kind, String) {

and the overload

 static func renderKindAndRole(_ kind: DocumentationNode.Kind?, linkSummary: LinkDestinationSummary?) -> (RenderNode.Kind, String) {

Then the common logic can be encapsulated in another function.

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.

4 participants