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

searchLocalStorageDataClasses #718

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

@apepper apepper requested a review from dcsaszar December 20, 2024 11:05
@dcsaszar dcsaszar self-assigned this Dec 27, 2024
Copy link
Contributor

@dcsaszar dcsaszar left a comment

Choose a reason for hiding this comment

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

Looks good so far. Depending on where we're heading with this, see FYI comments.

typeof value === 'string' &&
value.toLowerCase().includes(lowerCaseSearchTerm)

recordKeys.forEach(({ className: entity, recordKey }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • FYI we're mixing and renaming className and entity/entities for the same thing. Can we stick to either one or the other for better readability?

@@ -10,6 +10,8 @@ interface RawDataItem {
[key: string]: unknown
}

const recordKeys: Set<{ recordKey: string; className: string }> = new Set()
Copy link
Contributor

Choose a reason for hiding this comment

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

  •  FYI Making this a Set is misleading b/c the seemingly same key, e.g. recordKeys.add({ recordKey: "foo", className: "bar" }) will be added multiple times to the set. How about making it an Array instead?

@@ -25,6 +27,7 @@ export function provideLocalStorageDataClass(
} = {},
) {
const recordKey = `localDataClass-${scrivitoTenantId()}-${className}`
recordKeys.add({ className, recordKey })
Copy link
Contributor

Choose a reason for hiding this comment

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

  • FYI The recordKey can be recomputed easily from the className. How about reducing redundancy and just storing the list of class names?
  • FYI Taking it one step further: Looking at the intended usage the caller of searchLocalStorageDataClasses knows all class names. If this is true for all callers we could rid of the list of keys completely by taking the list of class names as an argument.

Object.entries(restoreRecord(recordKey)).forEach(([_id, item]) => {
if (Object.values(item).some(matchesSearchTerm)) {
results.push({
_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

  •  FYI We may end up with multiple results sharing the same _id, e.g., after copy-pasting to add a new local-storage data class. Not sure which implications this will have later on.

search: string,
blackListEntities: string[] = [],
): Array<{ _id: string; entity: string; title: string }> {
const results: Array<{ _id: string; entity: string; title: string }> = []
Copy link
Contributor

Choose a reason for hiding this comment

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

  •  FYI For a reader with a functional programming background something like this could be more readable (actually this is what I translate the logic of the function to in my head when reading the code):
  return entities.flatMap((entity) =>
    Object.entries(restoreRecord(recordKeyForClassName(entity)))
      .filter(([, item]) => Object.values(item).some(matchesSearchTerm))
      .map(([_id, item]) => ({
        _id,
        entity,
        title: ensureString(item.title) || ensureString(item.keyword),
      })),
  )

})
})

return results
Copy link
Contributor

Choose a reason for hiding this comment

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

  • FYI The results will be "sorted" by the runtime order of entities. Assuming an actual backend would sort by relevance, could we apply some sorting, e.g., by the number of matches per item?

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.

2 participants