Merged
Conversation
AdityaHegde
force-pushed
the
feat/small-improvements-to-search
branch
from
c8fac48 to
1b18781
Compare
ericpgreen2
approved these changes
Feb 21, 2025
Comment on lines
+63
to
+69
| data: values.filter((value) => { | ||
| if (seen.has(value)) { | ||
| return false; | ||
| } | ||
| seen.add(value); | ||
| return true; | ||
| }), |
Contributor
There was a problem hiding this comment.
Suggestion to remove this seen complexity:
const values = items.map((item) => item[dimensionName] as string);
// Convert to Set and back to array to remove duplicates
const data = [...new Set(values)];
return ...
| timeRange, | ||
| comparisonTimeRange, | ||
| measures, | ||
| limit: (maxValuesToShow - aboveTheFold.length).toString(), |
Contributor
There was a problem hiding this comment.
I'm wondering if we'd get a 400 if limit were 0 or negative. Maybe guard against limit <= 0 in the enabled clause?
Collaborator
Author
There was a problem hiding this comment.
It shouldnt happen, but doesnt hurt to add a safeguard. The limit is set to 100 if it was passed as 0, query would error out if it was <0.
| ); | ||
|
|
||
| return derived(queries, ($queries) => { | ||
| const someQueryFetching = $queries.some((q) => q.isFetching); |
Contributor
There was a problem hiding this comment.
Typically we should use isLoading not isFetching
Collaborator
Author
There was a problem hiding this comment.
Hmmm my understanding was isLoading is true only on the 1st fetch. But i guess it is for the 1st fetch of the query object. Since we create a new one on every searchText it will be true for showing the spinner.
Collaborator
Author
There was a problem hiding this comment.
But if for whatever reason there a refetch the data would go from blank to showing values without a spinner. So I think it is fine to use isFetching here. With tanstack v5 migration we wont need explicit compound query then we can revisit this.
| })), | ||
| }, | ||
| ]} | ||
| <!-- There will be some custom controls for this. Until we have the full design have a custom dropdown here. --> |
Contributor
There was a problem hiding this comment.
I'm a little worried about divergence between our menu components. For example, all other SearchableMenuContent would also benefit from the loading & error states you've added here. I wonder if we could add the requisite functionality to SearchableMenuComponent (e.g. make it pluggable?). I'll leave it up to you.
Collaborator
Author
There was a problem hiding this comment.
Ya I wanted to wait for the design to stabilise before refactoring this too much. The end goal would be a common component with hooks for dimension filter.
briangregoryholmes
added a commit
that referenced
this pull request
Feb 26, 2025
…6763) * fix(cloud-ui): redirect to login on expired session (#6733) * Safer redirect (homepage) * Safer redirect (CLI auth page) * Safer redirect (GitHub connect pages) * feat: public api for embeds (#6680) * feat: first rpc stub * add emit * feat: embed public api * fix: better name for state stream * fix: error handling and better spec conformatiy * fix: cleanup * fix: format * fix: make id optional * fix: first stub of tests * Revert "fix: first stub of tests" This reverts commit 2e33082. * fix: clear method names * fix: check input * fix: gitignore * fix: ignore * Revert "fix: ignore" This reverts commit 46ee2b3. * Revert "fix: gitignore" This reverts commit ec97c8e. * fix: add tests * Merge remote-tracking branch 'origin/main' into feat/emit-state-embed * fix: comments * fix: moving org and project to const * fix: moving embed to test hook * fix: format * fix: read token * fix: more fixes * fix: comments * fix: format * .gitignore * delete test file * fix: pr comments * fix: cleaner init * fix: format * fix: lint * fix: more lint (in my pocket) * fix: wait for bids * fix: format * fix: nits * fix: format * Fix accidentally skipped migration 0057.sql (#6737) * Canvas: add support for local time filters (#6673) * support individual metrics view for time controls * Add time controls for component * Create new helper method for filters * Working time filters * Match dimension filter toggle to mocks * Remove old filter references * Lint fix * Reset filters if metrics view is changed * Use local comparison range label for KPI * lint fix * Enable grid * Fix copy * Disable comparison for local filters by default * add image to concepts RD vs RC (#6729) * fix: Support CSV and Table output for `rill query` --format (#6741) * beautify csv and human output * add comments * Canvas: add stacked bar normalized (#6740) * Canvas: tweak image component to fit all image sizes (#6705) * Tweak: Do not show "New source" prompt if source has previously been ingested (#6616) * Do not show "New source" prompt if the source has already once been ingested * Add an "X" button in top-right of Alert Dialog * feat: small improvements to search (#6736) * Limit the max values in leaderboard to 15 * Add loading and error state for search * Fix lint * PR comments * fix: global properties should be set with global scope (#6746) * nit: trim semi colon in sql resolver (#6745) * trim semi colon in sql resolver * trim semi colon in sql resolver * fix: check for homedir on rill start (#6744) * fix: check for homedir on rill start * fix: random test failure * Revert "fix: random test failure" This reverts commit a8a13ec. * Use org name for autoscaler logs (#6747) * Use org name for autoscaler logs * Mark `dependency_error: true` for reconcile errors due to a dependency failure (#6732) * mark for reconcile errors due to a dependency failure * review * Update runtime/controller.go Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com> * review comments --------- Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com> * Add more context to billing and user logs (#6751) * tweak: rewrite kpi component, canvas cleanup (#6730) * wip * cleanup and layout adjustments * revert comment out * revert change * feedback * feedback * remove debug element * force ci * feat (Explore): Show the "last refreshed" date to everyone (#6750) * Integrate with new `modelRefreshedOn` property * Integrate with new `modelRefreshedOn` property (listings) * Delete unused function * Add comments * Fix null date * fix (microcopy): "source" -> "model" (#6754) * Fix microcopy: "source" -> "model" * Fix microcopy: "source" -> "model" * fix: do not select if shift key is used (#6758) * fix(explore): show error component not an infinite spinner (#6749) * fix (exports): handle filters and searches (#6742) * refactor: Push export logic into `ExportMenu` component * Rename `CreateScheduledReportDialog` -> `ScheduledReportDialog` * Handle scheduled reports * fix: Account for both filters and searches * Rename `scheduledReportsQueryArgs` -> `exportQueryArgs` * Remove wrapper function * Move fn to common location * Make function easier-to-test * Add unit tests * Self review * Fix time range for on-demand exports * Fix lint * Review * Review pt 2 * fix: Use cached plan details for page load to avoid calling orb's APIs (#6739) * Use cached plan details for page load * Replicate plan type detection in frontend * Fix lint * Fix race condition in showing billing banner * Fix all time in yaml crashing the app (#6760) * Add organization billing plan name to default deployment annotations (#6761) * init commit * cleanup * remove import * fix (Cloud UI): Always redirect to login when auth token expires (#6766) * Catch expired tokens in root `+layout.ts` * Use native SvelteKit redirect (to avoid error page flash) * e2e: Capture logs from Go services (#6771) * Save `admin` and `runtime` output to `playwright/logs/admin-runtime.log` * Upload logfile in GitHub Action * fix panic due to uri parse error (#6772) * remove log * remove log * add comment * always show outline around selected components, hide comparison line in KPI * reorg, cleanup and safari fixes * type fix --------- Co-authored-by: Eric P Green <ericpgreen2@gmail.com> Co-authored-by: Alexander Thor <alec.karlsson@gmail.com> Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com> Co-authored-by: Dhiraj Barnwal <sumankumaribarnwal@gmail.com> Co-authored-by: Roy Endo <67675319+royendo@users.noreply.github.com> Co-authored-by: Graham Plata <graham.plata@gmail.com> Co-authored-by: Aditya Hegde <adityahegderocks@gmail.com> Co-authored-by: Anshul Khandelwal <12948312+k-anshul@users.noreply.github.com> Co-authored-by: Himadri Singh <himadri.singh@gmail.com> Co-authored-by: Kasper Sjørslev <kaspersjo@users.noreply.github.com>
begelundmuller
added a commit
that referenced
this pull request
Feb 27, 2025
* Implement `rows:` in the canvas YAML * Self review * Fix test * Potential fix for code scanning alert no. 31: Incorrect conversion between integer types Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * Optional width/height * Flatten inline component definitions * Fix Go test * feat: visual canvas editor yaml structure integration and refinement (#6763) * fix(cloud-ui): redirect to login on expired session (#6733) * Safer redirect (homepage) * Safer redirect (CLI auth page) * Safer redirect (GitHub connect pages) * feat: public api for embeds (#6680) * feat: first rpc stub * add emit * feat: embed public api * fix: better name for state stream * fix: error handling and better spec conformatiy * fix: cleanup * fix: format * fix: make id optional * fix: first stub of tests * Revert "fix: first stub of tests" This reverts commit 2e33082. * fix: clear method names * fix: check input * fix: gitignore * fix: ignore * Revert "fix: ignore" This reverts commit 46ee2b3. * Revert "fix: gitignore" This reverts commit ec97c8e. * fix: add tests * Merge remote-tracking branch 'origin/main' into feat/emit-state-embed * fix: comments * fix: moving org and project to const * fix: moving embed to test hook * fix: format * fix: read token * fix: more fixes * fix: comments * fix: format * .gitignore * delete test file * fix: pr comments * fix: cleaner init * fix: format * fix: lint * fix: more lint (in my pocket) * fix: wait for bids * fix: format * fix: nits * fix: format * Fix accidentally skipped migration 0057.sql (#6737) * Canvas: add support for local time filters (#6673) * support individual metrics view for time controls * Add time controls for component * Create new helper method for filters * Working time filters * Match dimension filter toggle to mocks * Remove old filter references * Lint fix * Reset filters if metrics view is changed * Use local comparison range label for KPI * lint fix * Enable grid * Fix copy * Disable comparison for local filters by default * add image to concepts RD vs RC (#6729) * fix: Support CSV and Table output for `rill query` --format (#6741) * beautify csv and human output * add comments * Canvas: add stacked bar normalized (#6740) * Canvas: tweak image component to fit all image sizes (#6705) * Tweak: Do not show "New source" prompt if source has previously been ingested (#6616) * Do not show "New source" prompt if the source has already once been ingested * Add an "X" button in top-right of Alert Dialog * feat: small improvements to search (#6736) * Limit the max values in leaderboard to 15 * Add loading and error state for search * Fix lint * PR comments * fix: global properties should be set with global scope (#6746) * nit: trim semi colon in sql resolver (#6745) * trim semi colon in sql resolver * trim semi colon in sql resolver * fix: check for homedir on rill start (#6744) * fix: check for homedir on rill start * fix: random test failure * Revert "fix: random test failure" This reverts commit a8a13ec. * Use org name for autoscaler logs (#6747) * Use org name for autoscaler logs * Mark `dependency_error: true` for reconcile errors due to a dependency failure (#6732) * mark for reconcile errors due to a dependency failure * review * Update runtime/controller.go Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com> * review comments --------- Co-authored-by: Benjamin Egelund-Müller <b@egelund-muller.com> * Add more context to billing and user logs (#6751) * tweak: rewrite kpi component, canvas cleanup (#6730) * wip * cleanup and layout adjustments * revert comment out * revert change * feedback * feedback * remove debug element * force ci * feat (Explore): Show the "last refreshed" date to everyone (#6750) * Integrate with new `modelRefreshedOn` property * Integrate with new `modelRefreshedOn` property (listings) * Delete unused function * Add comments * Fix null date * fix (microcopy): "source" -> "model" (#6754) * Fix microcopy: "source" -> "model" * Fix microcopy: "source" -> "model" * fix: do not select if shift key is used (#6758) * fix(explore): show error component not an infinite spinner (#6749) * fix (exports): handle filters and searches (#6742) * refactor: Push export logic into `ExportMenu` component * Rename `CreateScheduledReportDialog` -> `ScheduledReportDialog` * Handle scheduled reports * fix: Account for both filters and searches * Rename `scheduledReportsQueryArgs` -> `exportQueryArgs` * Remove wrapper function * Move fn to common location * Make function easier-to-test * Add unit tests * Self review * Fix time range for on-demand exports * Fix lint * Review * Review pt 2 * fix: Use cached plan details for page load to avoid calling orb's APIs (#6739) * Use cached plan details for page load * Replicate plan type detection in frontend * Fix lint * Fix race condition in showing billing banner * Fix all time in yaml crashing the app (#6760) * Add organization billing plan name to default deployment annotations (#6761) * init commit * cleanup * remove import * fix (Cloud UI): Always redirect to login when auth token expires (#6766) * Catch expired tokens in root `+layout.ts` * Use native SvelteKit redirect (to avoid error page flash) * e2e: Capture logs from Go services (#6771) * Save `admin` and `runtime` output to `playwright/logs/admin-runtime.log` * Upload logfile in GitHub Action * fix panic due to uri parse error (#6772) * remove log * remove log * add comment * always show outline around selected components, hide comparison line in KPI * reorg, cleanup and safari fixes * type fix --------- Co-authored-by: Brian Holmes <120223836+briangregoryholmes@users.noreply.github.com>
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.
Small independent items from https://www.notion.so/rilldata/Bulk-and-advanced-filtering-196ba33c8f5780b4b25dce617ce55ed6?d=1a0ba33c8f5780d4b243001cf434ea50#19fba33c8f5780da9dbff8d641520c92
Checklist: