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.

Sign up for GitHub

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

Jump to bottom

Fix/query performance #9016

Merged
merged 5 commits into from Apr 7, 2023
Merged

Fix/query performance #9016

merged 5 commits into from Apr 7, 2023

Conversation

tiensonqin
Copy link
Contributor

@tiensonqin tiensonqin commented Apr 3, 2023 •
edited

This PR fixed:

  1. don't trigger queries multiple when rendering, it can be caused by hovering the foldable title for custom queries
  2. schedule and deadlines no longer work.

@tiensonqin tiensonqin marked this pull request as ready for review April 4, 2023 06:09
andelf
andelf approved these changes Apr 4, 2023
View reviewed changes
Copy link
Collaborator

@andelf andelf left a comment

Choose a reason for hiding this comment

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

LGTM

@tiensonqin tiensonqin mentioned this pull request Apr 4, 2023
Merged
logseq-cldwalker
logseq-cldwalker approved these changes Apr 6, 2023
View reviewed changes
Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

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

@tiensonqin Confirmed that deadline/scheduled works 👍 🚢 I wasn't able to reproduce the hovering title issue but I trust you were fixing something. Would be nice to have some test with query triggering sometime as there are no existing tests so we run the risk of introducing more breakages each time

src/main/frontend/components/block.cljs
@@ -3117,38 +3120,45 @@
{:on-mouse-down on-mouse-down}
(ui/icon "refresh" {:style {:font-size 20}})]))

(defn- get-query-result
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

src/main/frontend/state.cljs Outdated
@@ -496,7 +496,7 @@ should be done through this fn in order to get global config and config defaults
(defn get-scheduled-future-days
[]
(let [days (:scheduled/future-days (get-config))]
(or (when (int? days) days) 0)))
(or (when (int? days) days) 14)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing the default may surprise people as it's on the homepage. Should we mention it in the release note somehow? There's a related comment in the default config to update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated it back to 7.

@tiensonqin
Copy link
Contributor Author

@logseq-cldwalker

I wasn't able to reproduce the hovering title issue but I trust you were fixing something.

Try sample which can return different result each time.

@tiensonqin tiensonqin merged commit bdc15d0 into master Apr 7, 2023
6 checks passed
@tiensonqin tiensonqin deleted the fix/query-performance branch April 7, 2023 06:13
logseq-cldwalker added a commit that referenced this pull request Apr 7, 2023
@logseq-cldwalker
Update comment to reflect update in #9016
9bf867e
@andelf andelf mentioned this pull request Apr 12, 2023
Closed
8 tasks
bendyorke pushed a commit that referenced this pull request May 9, 2023
@logseq-cldwalker @bendyorke
Update comment to reflect update in #9016
fc23b12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andelf andelf andelf approved these changes

@logseq-cldwalker logseq-cldwalker logseq-cldwalker approved these changes

@cnrpman cnrpman Awaiting requested review from cnrpman

@RCmerci RCmerci Awaiting requested review from RCmerci

Assignees
No one assigned
Labels
:type/bug-fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tiensonqin @andelf @logseq-cldwalker