Repository navigation
Conversation
- Add a "Sort by" dropdown to Search with Relevance (default), Newest first, Oldest first, Creation date and Most discussed. The four orders are shared with the discussions feed, which also gains Most discussed. - Allow searching with filters only; without a query results come back newest first. - Sort inside the SQLite query so the order applies before the result limit. The index stores creation and comments_count for this, so a patch drops the old index and it is rebuilt after migrate. - Break ties on the feed by name so paging through Most discussed does not skip or repeat threads. - Show at most 100 options in the Author, Space and Tags filters, with the rest found by typing and selected options pinned to the top. Select All still selects every option. - Add Reset filters, clamp result text to two lines with the match kept in view, keep the result summary above the toolbar fade, and restore a filters-only search when navigating back. - Raise GameplanSearchIndexMissingError when the index is missing so the Search page and command palette handle it, and show a readable message while the index is rebuilt.
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Keep only the owner, project, team, doctype and tags filters before a search query is built. Filter names are part of the SQL text, so an arbitrary name could rewrite the WHERE clause and return results from private spaces. - Build the new search index inside the patch that drops the old one, so search is available as soon as migrate finishes instead of waiting for a background worker.
Comment on lines
+424
to
+428
| sql = f""" | ||
| SELECT {select_clause} | ||
| FROM search_fts | ||
| {where_clause} | ||
| ORDER BY CAST({sort_column} AS REAL) {sort_direction.upper()} |
Contributor
There was a problem hiding this comment.
Context Used: Guidelines for reviewing Frappe Framework applications. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: gameplan/search_sqlite.py
Line: 424-428
Comment:
**Interpolated SQL query** The sorted search builds SQL with an f-string, inserting query fragments directly into the statement. This violates the repository instruction to use a query builder or parameterized raw SQL without interpolated SQL fragments. Please satisfy that requirement before merging.
**Context Used:** Guidelines for reviewing Frappe Framework applications. ([source](https://github.com/frappe/skills/blob/main/skills/quality-code-review/SKILL.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| sort_column, sort_direction = self._parse_sort_by(DEFAULT_SORT_BY) | ||
|
|
||
| if sort_column: | ||
| return self._sorted_search(query, title_only, filters, sort_column, sort_direction) |
Contributor
There was a problem hiding this comment.
this is hacky sorting, should override native frappe sqlite search sorting
- Show Relevance as the starting value of Sort by and list it among the options once another order has been picked, without the "(default)" suffix. - Rename Reset filters to Clear filters. - Give the Author filter's chevron the same colour as the other filters.
…path - Send every search with a query through SQLiteSearch.search and override its query and result steps for the chosen order, instead of rebuilding the whole search in _sorted_search. - Relevance searches run frappe's query unchanged; other orders replace only the ORDER BY step and restore that order after frappe's scoring. - Filters-only searches reuse the same two steps, since frappe's search returns early on an empty query.
| else: | ||
| select_fields.append("content") | ||
|
|
||
| select_fields.extend(field for field in self.schema["metadata_fields"] if field != "doc_id") |
Contributor
There was a problem hiding this comment.
modified. The Search page then shows invalid dates, and “Newest first” and “Oldest first” cannot put results in timestamp order after result processing. Include modified in the sorted results.
Prompt To Fix With AI
This is a comment left during a code review.
Path: gameplan/search_sqlite.py
Line: 478
Comment:
**Sorted results lose timestamps** When someone searches with filters alone or selects a sort order, this SELECT omits `modified`. The Search page then shows invalid dates, and “Newest first” and “Oldest first” cannot put results in timestamp order after result processing. Include `modified` in the sorted results.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
search_redesign.mp4