Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add search functionality to Topics view #1010

Merged
merged 22 commits into from
Feb 5, 2025
Merged

Conversation

shouples
Copy link
Contributor

@shouples shouples commented Feb 3, 2025

Summary of Changes

Closes #1002.

The Topics view is now searchable with the same general mechanisms as the Resources view search done in #996, specifically to allow searching by topic name and schema subject.

topics-view-search.mov

Any additional details or context that should be provided?

Highlighting this part specifically:

// HACK: we need to create children to allow searching the Topics view by schema subject; this
// will only allow the topic to "match" and be returned from the TopicsViewProvider's
// `getChildren()` method, and then once the topic is expanded, it will show the real subject
// container(s), which will be the source of truth for subjects+schemas
const subjectChildren: ContainerTreeItem<Schema>[] = matchingSubjects.map(
(subject) => new ContainerTreeItem<Schema>(subject, TreeItemCollapsibleState.Collapsed, []),
);

The search logic is fundamentally about what items are returned at each level of the tree data. The topics view is responsible for two main things upon loading at the root level: listing topics, and listing schema subjects to decorate the topics. Based on those two facts, the above snippet ensures we can search for a subject by preemptively setting a topic's children to be empty subject containers. If a Kafka topic matches based on its subject, it can then go through the normal motions of expanding to show the real subject container here:

// NOTE: we end up here when expanding a (collapsed) treeItem
if (element instanceof KafkaTopic) {
// return schema-subject containers
children = await loadTopicSchemas(element);

...which we then are able to expand further and see all associated schema versions due to the subject container matching:

// if the parent item matches the search string, return all children so the user can expand
// and see them all, even if just the parent item matched and shows the highlight(s)
const parentMatched = element && itemMatchesSearch(element, this.itemSearchString);

All this to say: we can't search by any other pieces of information about a schema unless we eagerly load them and make sure they're available in the earlier-mentioned-empty-container-hack within loaderUtils.ts.

Pull request checklist

Please check if your PR fulfills the following (if applicable):

Tests
  • Added new
  • Updated existing
  • Deleted existing
Other
  • All new disposables (event listeners, views, channels, etc.) collected as for eventual cleanup?
  • Does anything in this PR need to be mentioned in the user-facing CHANGELOG or README?
  • Have you validated this change locally by packaging and installing the extension .vsix file?
    gulp clicktest

@shouples shouples linked an issue Feb 4, 2025 that may be closed by this pull request
Base automatically changed from djs/search-primary-views to main February 4, 2025 14:49
return `${this.subject} ${this.version}`;
return this.id;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This mainly ensures we don't over-match while searching for a subject. It isn't currently possible to search for a schema ID in the Topics view since we don't have any eagerly-fetched Schemas to attach to the subject container tree items, just the subjects themselves.

Comment on lines +136 to 137
// @ts-expect-error we need to update @types/vscode to make ThemeColor.id public
assert.strictEqual((icon as vscode.ThemeIcon).color!.id, "problemsWarningIcon.foreground");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +27 to +28
// set id to the label so it isn't `undefined`; can be overwritten by the caller if needed
this.id = label.toString();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shouples shouples marked this pull request as ready for review February 4, 2025 22:38
@shouples shouples requested a review from a team as a code owner February 4, 2025 22:38
@@ -31,7 +31,7 @@ export const TEST_LOCAL_KAFKA_TOPIC: KafkaTopic = KafkaTopic.create({
name: "test-local-topic",
partition_count: 1,
clusterId: TEST_LOCAL_KAFKA_CLUSTER.id,
environmentId: LOCAL_CONNECTION_ID,
environmentId: TEST_LOCAL_KAFKA_CLUSTER.environmentId,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated to this PR: another EnvironmentId branded type fix

src/models/topic.ts Outdated Show resolved Hide resolved
@shouples shouples merged commit 4cab0bd into main Feb 5, 2025
2 checks passed
@shouples shouples deleted the djs/search-topics-view branch February 5, 2025 16:17
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.

Add search functionality to the Topics view
2 participants