Summary:
Adds initial implementation of dark mode. Preferences for dark mode are
available on the account settings page. Dark mode is currently a beta feature
as there are still some small bugs to squash and a few remaining UI elements
to style.
Test Plan: Browser tests.
Reviewers: jarek
Reviewed By: jarek
Subscribers: paulfitz, jarek
Differential Revision: https://phab.getgrist.com/D3587
Summary:
- The issue manifested as error "Cannot read property '0' of undefined" in some
cases, and as "Blocked by table read access rules" in others (instead of
limiting output to what's not blocked)
- Goes deeper: exports weren't respecting metadata censoring.
- The fix changes exports to use censored metadata, which addresses both errors above.
- Includes an improvement to column ordering in XLSX exports.
Test Plan: Add a server test for CSV and XLSX exports with access rules
Reviewers: paulfitz, georgegevoian
Reviewed By: paulfitz, georgegevoian
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3615
Summary:
Fixes some bugs involving intervals, and updates RandomizedTimer to support both fixed and
randomized delays, and to better handle async callbacks.
* Fixed a bug where Throttle would queue up many pidusage calls due to the use of
setInterval, and the async nature of the calls.
* Fixed a but where RandomizedTimer (now just Interval) would not be disabled in
ActiveDoc on doc shutdown if initialization had not yet settled.
Test Plan: Tested manually.
Reviewers: jarek, dsagal
Reviewed By: jarek, dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3604
Summary:
Show raw data will now open a popup with
raw section instead of redirecting to raw data page.
Adding new anchor link type "a2" that is able to open
any section in a popup on a current view.
Not related:
Fixing highlightMatches function, after merging core PR.
Test Plan: Updated tests
Reviewers: alexmojaki, georgegevoian
Reviewed By: alexmojaki, georgegevoian
Subscribers: georgegevoian, alexmojaki
Differential Revision: https://phab.getgrist.com/D3592
Summary:
When large numbers of documents were restarted simultaneously, they had
a tendency to schedule intervals to begin at roughly the same moment in
time, causing periodic spikes in load. This randomizes the delay of each
interval to help avoid such spikes.
Test Plan: Tested manually.
Reviewers: alexmojaki
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3600
Summary:
Adds an activation page to grist-ee that currently shows activation status.
Follow-up diffs will introduce additional functionality, such as the ability to
enter activation keys directly from the activation page.
Test Plan: No grist-ee tests (yet).
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: dsagal, paulfitz
Differential Revision: https://phab.getgrist.com/D3582
Summary:
- Moved /test/client and /test/common to core.
- Moved two files (CircularArray and RecentItems) from app/common to core/app/common.
- Moved resetOrg test to gen-server.
- `testrun.sh` is now invoking common and client test from core.
- Added missing packages to core's package.json (and revealed underscore as it is used in the main app).
- Removed Coord.js as it is not used anywhere.
Test Plan: Existing tests
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3590
Summary:
Conditional formatting can now be used for whole rows.
Related fix:
- Font styles weren't applicable for summary columns.
- Checkbox and slider weren't using colors properly
Test Plan: Existing and new tests
Reviewers: paulfitz, georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3547
Summary:
Removing NEW_DEAL flag checks and exposing all new deal features as default.
Also removing Pages.ts as it was moved to grist-core.
Test Plan: Existing and updated tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3572
Summary: Displays a live row count of each table on the Raw Data page.
Test Plan: Browser tests.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3540
Summary: Adding new url parameter for team site creation
Test Plan: Updated tests.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3554
Summary:
Adds the new personal plan as a product that will be available
in the future. Can be enabled along with other plan-related via
an environment variable.
Test Plan: Browser tests and existing tests.
Reviewers: jarek
Reviewed By: jarek
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3533
Summary: [Select By] in the creator panel was bugged. It wasn't refreshed in some cases as the observable array that needed to be created seemed too complicated. This Diff recomputes this array when the user wants to change the selection.
Test Plan: added tests
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3541
Summary:
As reported in https://grist.slack.com/archives/C069RUP71/p1655316194602829, when a table is hidden by ACL, it was still showing as a blank 'ghost' option to select data when adding a widget:
{F55498}
The fix is simply to return `true` from `isHiddenTable` for empty table IDs, which indicate a table hidden by ACL. `TableRec.isHidden` is supposed to match this so I updated it too, and I cleaned up a tiny bit of other related code.
Test Plan: Extended `nbrowser/AccessRules1.ts` to test the data options when adding widgets.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3530
Summary:
With this, a custom widget can render an attachment by doing:
```
const tokenInfo = await grist.docApi.getAccessToken({readOnly: true});
const img = document.getElementById('the_image');
const id = record.C[0]; // get an id of an attachment
const src = `${tokenInfo.baseUrl}/attachments/${id}/download?auth=${tokenInfo.token}`;
img.setAttribute('src', src)
```
The access token expires after a few mins, so if a user right-clicks on an image
to save it, they may get access denied unless they refresh the page. A little awkward,
but s3 pre-authorized links behave similarly and it generally isn't a deal-breaker.
Test Plan: added tests
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3488
Summary:
Use table titles (i.e. the raw data widget titles) in dropdowns and other parts of the Acess Rules page, instead of the table ID. This is particularly meant for summary tables which have/had an ID of the form `GristSummary_SourceTable_N`, but https://phab.getgrist.com/D3508 is changing that anyway.
The server method `getAclResources` now returns more metadata about each table so that the UI can display titles.
Test Plan: Extended and updated `nbrowser/AccessRules2.ts`. Added a small unit test for constructing table titles from the new description returned by `getAclResources`.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3494
Summary:
Improving billing page user experience.
- Updated labels for canceled plan
- Adding option to downgrade from team plan to free team plan
- Updating default name for teamFree plan when it is not available in Stripe
- Minor bug fixes
Test Plan: updated tests
Reviewers: cyprien
Reviewed By: cyprien
Subscribers: cyprien
Differential Revision: https://phab.getgrist.com/D3515
Summary:
Changes auto-generated summary table IDs from e.g. `GristSummary_6_Table1` to `Table1_summary_A_B` (meaning `Table1` grouped by `A` and `B`). This makes it easier to write formulas involving summary tables, make API requests, understand logs, etc.
Because these don't encode the source table ID as reliably as before, `decode_summary_table_name` now uses the summary table schema info, not just the summary table ID. Specifically, it looks at the type of the `group` column, which is `RefList:<source table id>`.
Renaming a source table renames the summary table as before, and now renaming a groupby column renames the summary table as well.
Conflicting table names are resolved in the usual way by adding a number at the end, e.g. `Table1_summary_A_B2`. These summary tables are not automatically renamed when the disambiguation is no longer needed.
A new migration renames all summary tables to the new scheme, and updates formulas using summary tables with a simple regex.
Test Plan:
Updated many tests to use the new style of name.
Added new Python tests to for resolving conflicts when renaming source tables and groupby columns.
Added a test for the migration, including renames in formulas.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3508
Summary:
When a user requests to read the contents of an attachment, only allow the request if there exists a cell in an attachment column that contains the attachment and which they have read access to.
This does not cover:
* Granular write access for attachments. In particular, a user who can write to any attachment column should be considered to have full read access to all attachment columns, currently.
* Access control of attachment metadata such as name and format.
The implementation uses a sql query that requires a scan, and some notes on how this could be optimized in future. The web client was updated to specify the cell to check for access, and performance seemed fine in casual testing on a doc with 1000s of attachments. I'm not sure how performance would hold up as the set of access rules grows as well.
Test Plan: added tests
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3490
Summary:
Summary tables now have their own raw viewsection, and are shown
under Raw Data Tables on the Raw Data page.
Test Plan: Browser and Python tests.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3495
Summary:
Building:
- Builds no longer wait for tsc for either client, server, or test targets. All use esbuild which is very fast.
- Build still runs tsc, but only to report errors. This may be turned off with `SKIP_TSC=1` env var.
- Grist-core continues to build using tsc.
- Esbuild requires ES6 module semantics. Typescript's esModuleInterop is turned
on, so that tsc accepts and enforces correct usage.
- Client-side code is watched and bundled by webpack as before (using esbuild-loader)
Code changes:
- Imports must now follow ES6 semantics: `import * as X from ...` produces a
module object; to import functions or class instances, use `import X from ...`.
- Everything is now built with isolatedModules flag. Some exports were updated for it.
Packages:
- Upgraded browserify dependency, and related packages (used for the distribution-building step).
- Building the distribution now uses esbuild's minification. babel-minify is no longer used.
Test Plan: Should have no behavior changes, existing tests should pass, and docker image should build too.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3506
Summary:
- Update nudge boxes content and collapsing on personal and free team site
- New confirmation after upgrading from a free team site
- Refactoring ProductUpgrade code, splitting plans / modals and nudges
Test Plan: Manual and updated tests
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3481
Summary:
- Upgrades to build-related packages:
- Upgrade typescript, related libraries and typings.
- Upgrade webpack, eslint; add tsc-watch, node-dev, eslint_d.
- Build organization changes:
- Build webpack from original typescript, transpiling only; with errors still
reported by a background tsc watching process.
- Typescript-related changes:
- Reduce imports of AWS dependencies (very noticeable speedup)
- Avoid auto-loading global @types
- Client code is now built with isolatedModules flag (for safe transpilation)
- Use allowJs to avoid copying JS files manually.
- Linting changes
- Enhance Arcanist ESLintLinter to run before/after commands, and set up to use eslint_d
- Update eslint config, and include .eslintignore to avoid linting generated files.
- Include a bunch of eslint-prompted and eslint-generated fixes
- Add no-unused-expression rule to eslint, and fix a few warnings about it
- Other items:
- Refactor cssInput to avoid circular dependency
- Remove a bit of unused code, libraries, dependencies
Test Plan: No behavior changes, all existing tests pass. There are 30 tests fewer reported because `test_gpath.py` was removed (it's been unused for years)
Reviewers: paulfitz
Reviewed By: paulfitz
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3498
Summary:
This diff is first of a series of 3 commits to enable range filering
for Date and DateTime columns. Diff only enable setting date's min/max
throw typing dates, Date picker and relative ranges are left for
follow-up commits.
- Exposes columns value formatter to the range input
- Fixes column filter func to work with dates
Test Plan:
Adds Date to projects range filter test
Adds Date/DateTime to nbrowser ColumnFilterMenu tests
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3455
Summary:
Adds a Python function `REQUEST` which makes an HTTP GET request. Behind the scenes it:
- Raises a special exception to stop trying to evaluate the current cell and just keep the existing value.
- Notes the request arguments which will be returned by `apply_user_actions`.
- Makes the actual request in NodeJS, which sends back the raw response data in a new action `RespondToRequests` which reevaluates the cell(s) that made the request.
- Wraps the response data in a class which mimics the `Response` class of the `requests` library.
In certain cases, this asynchronous flow doesn't work and the sandbox will instead synchronously call an exported JS method:
- When reevaluating a single cell to get a formula error, the request is made synchronously.
- When a formula makes multiple requests, the earlier responses are retrieved synchronously from files which store responses as long as needed to complete evaluating formulas. See https://grist.slack.com/archives/CL1LQ8AT0/p1653399747810139
Test Plan: Added Python and nbrowser tests.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: paulfitz, dsagal
Differential Revision: https://phab.getgrist.com/D3429
Summary:
After the welcome questions are dismissed, a video tour modal will
now be displayed. The video tour is also accessible via a tool button
in the left panel of the home page, as well as a text button next to
the Examples & Templates header.
Test Plan: Browser tests.
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3477
Summary:
- Substantial refactoring of the logic when the server fails to send some
messages to a client.
- Add seqId numbers to server messages to ensure reliable order.
- Add a needReload flag in clientConnect for a clear indication whent the
browser client needs to reload the app.
- Reproduce some potential failure scenarios in a test case (some of which
previously could have led to incorrectly ordered messages).
- Convert other Comm tests to typescript.
- Tweak logging of Comm and Client to be slightly more concise (in particular,
avoid logging sessionId)
Note that despite the big refactoring, this only addresses a fairly rare
situation, with websocket failures while server is trying to send to the
client. It includes no improvements for failures while the client is sending to
the server.
(I looked for an existing library that would take care of these issues. A relevant article I found is https://docs.microsoft.com/en-us/azure/azure-web-pubsub/howto-develop-reliable-clients, but it doesn't include a library for both ends, and is still in review. Other libraries with similar purposes did not inspire enough confidence.)
Test Plan: New test cases, which reproduce some previously problematic scenarios.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3470
Summary:
For self-hosted Grist, forward auth has proven useful, where
some proxy wrapped around Grist manages authentication, and
passes on user information to Grist in a trusted header.
The current implementation is adequate when Grist is the
only place where the user logs in or out, but is confusing
otherwise (see https://github.com/gristlabs/grist-core/issues/207).
Here we take some steps to broaden the scenarios Grist's
forward auth support can be used with:
* When a trusted header is present and is blank, treat
that as the user not being logged in, and don't look
any further for identity information. Specifically,
don't look in Grist's session information.
* Add a `GRIST_IGNORE_SESSION` flag to entirely prevent
Grist from picking up identity information from a cookie,
in order to avoid confusion between multiple login methods.
* Add tests for common scenarios.
Test Plan: added tests
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3482
Summary:
A formula returning an empty RecordSet in a RefList columns results in storing [] instead of null.
This caused a bug where the empty list was 'flattened' and the cell not appearing in filters at all.
This diff fixes the bug by filtering for the default value `null` instead for RefLists and the empty string for ChoiceLists.
I didn't manage to actually reproduce the bug for ChoiceLists, but this seemed the most sensible thing to do.
Test Plan: New nbrowser test.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3478
Summary:
Adds some special handling to summary table and lookup logic:
- Source rows with empty choicelists/reflists get a corresponding summary row with an empty string/reference when grouping by that column, instead of excluding them from any group
- Adds a new `QueryOperation` 'empty' in the client which is used in `LinkingState`, `QuerySet`, and `recursiveMoveToCursorPos` to match empty lists in source tables against falsy values in linked summary tables.
- Adds a new parameter `match_empty` to the Python `CONTAINS` function so that regular formulas can implement the same behaviour as summary tables. See https://grist.slack.com/archives/C0234CPPXPA/p1654030490932119
- Uses the new `match_empty` argument in the formula generated for the `group` column when detaching a summary table.
Test Plan: Updated and extended Python and nbrowser tests of summary tables grouped by choicelists to test for new behaviour with empty lists.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3471
Summary:
- Showing nudge to individual users to sign up for free team plan.
- Implementing billing page to upgrade from free team to pro.
- New modal with upgrade options and free team site signup.
- Integrating Stripe-hosted UI for checkout and plan management.
Test Plan: updated tests
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3456
Summary:
- Add app/common/CommTypes.ts to define types shared by client and server.
- Include @types/ws npm package
Test Plan: Intended to have no changes in behavior
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3467
Summary:
If an external store fails completely, Grist will continue to
retry uploading to it. This diff updated the HostedStorageManager
test to limit the extent of these retries to the test itself -
otherwise they continue for all other tests in the same process,
potentially disrupting those that read logs. There are other tests
that use s3, but they aren't run in the same process with delicate
log-reading tests, and it isn't quite as clear what improvement
to make there.
Test Plan:
artificially made external store fail, and checked that
test contamination seen previously no longer occurs.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3469
Summary:
Instead of always redirecting new users to the home page or the (teams) welcome page,
only redirect when the user signed in for the first time on a personal site, has access to
other sites, and isn't already being redirected to a specific page on their personal site.
Also tweaks how invalid Choice column values are displayed to match Choice List
columns, and fixes a small CSS issue with select by in the page widget picker when
there are options with long labels.
Test Plan: Browser tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3461
Summary:
Introduces a new message type, docUsage, that's broadcast to all connected
clients whenever document usage is updated in ActiveDoc.
Test Plan: Browser tests.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3451
Summary:
- Remove the empty-folder icon
- Add an "Invite team members" button for owners on empty team sites
- Add a "Browse Templates" button for all other cases on empty sites
- Update intro text for team, including a link to Sprouts
- Update intro text for personal/anon.
- Include a Free/Pro tag for team sites (for now, only "Free")
- Add a "Manage Team" button for owners on home page of all team sites.
- Polished the UI of UserManager: add a transition for the background, and
delay the appearance of the spinner for fast loads.
Test Plan: Fixed up the HomeIntro tests; added test case for Manage Team button
Reviewers: georgegevoian
Reviewed By: georgegevoian
Differential Revision: https://phab.getgrist.com/D3459
Summary:
Shows the range filter next to the filter by values on filter menu. When users
set min and/or max, it takes precendence over the filter by values.
If users set:
- `[] < [max]` behaves as `less than max`.
- `[min] < []` behaves as `more than min`.
- `[min] < [max]` behaves as `between min and max`
- bounds are always inclusives.
- when users change min or max the values of the by values filter
gets checked/unchecked depending on whether they are included by
the range filter.
- when users clicks any btn/checkbox of the by values filter both min
and max input gets cleared, and the filter convert to a filter by
values.
Test Plan: Adds both projets and nbrowser tests.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3435
Summary:
Also fixes a minor CSS regression in UserManager where the
link to add a team member wasn't shown on a separate row.
Test Plan: Browser tests.
Reviewers: jarek
Reviewed By: jarek
Differential Revision: https://phab.getgrist.com/D3444
Summary:
Previously, columns of type Any were created and modified one by one by reusing
the "empty column" logic from the data engine. This copies that logic to Node,
and sets the type of all columns together, to create them with the correct type
in the AddTable call.
This makes imports about twice faster (when slowness is due to many columns),
but doesn't address all cases where individual handling of columns causes slowness.
Test Plan: Added a test case for the new helper function.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3427
Summary:
Instead of showing a blank dialog for users whose access
is limited (e.g. public members), we now show the user's
role and a mention of whether their access is public.
Test Plan: Browser tests.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3431
Summary:
This makes it possible to configure a SendGrid-based Notifier
instance via a JSON configuration file.
Test Plan: Tested manually.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: paulfitz
Differential Revision: https://phab.getgrist.com/D3432
Summary: For grist-ee, expect an activation key in environment variable `GRIST_ACTIVATION` or in a file pointed to by `GRIST_ACTIVATION_FILE`. In absence of key, start a 30-day trial, during which a banner is shown. Once trial expires, installation goes into document-read-only mode.
Test Plan: added a test
Reviewers: dsagal
Reviewed By: dsagal
Subscribers: jarek
Differential Revision: https://phab.getgrist.com/D3426
Summary:
The summary includes a count of documents that are approaching
limits, in grace period, or delete-only. The endpoint is only accessible
to site owners, and is currently unused. A follow-up diff will add usage
banners to the site home page, which will use the response from the
endpoint to communicate usage information to owners.
Test Plan: Browser and server tests.
Reviewers: alexmojaki
Reviewed By: alexmojaki
Differential Revision: https://phab.getgrist.com/D3420
Summary:
Adds a new implementation of the interface ExternalStorage that works for Azure Blob Storage as an alternative to S3, for a specific self-hosting case.
Tweaks HostedStorageManager and ICreate to allow configuring different core implementations of ExternalStorage.
Followup tasks:
- Make this code available to self hosters, possibly by making it open source.
- Add an env var or other config option to specify the preferred type of storage. Currently using the var `AZURE_STORAGE_CONNECTION_STRING` to know how to connect to Azure when requested, but that choice still only lives in test code.
Test Plan: Generalized HostedStorageManager and ExternalStorage tests to test the new AzureExternalStorage alongside S3ExternalStorage. The HostedStorageManager tests also now test the 'cached' in-memory test storage in a way that's closer to the real storage methods.
Reviewers: paulfitz
Reviewed By: paulfitz
Differential Revision: https://phab.getgrist.com/D3413
Summary:
Adds attachment and data size to the usage section of
the raw data page. Also makes in-document usage banners
update as user actions are applied, causing them to be
hidden/shown or updated based on the current state of
the document.
Test Plan: Browser tests.
Reviewers: jarek
Reviewed By: jarek
Subscribers: alexmojaki
Differential Revision: https://phab.getgrist.com/D3395
Summary:
A new way for renaming tables.
- There is a new popup to rename section (where you can also rename the table)
- Renaming/Deleting page doesn't modify/delete the table.
- Renaming table can rename a page if the names match (and the page contains a section with that table).
- User can rename table in Raw Data UI in two ways - either on the listing or by using the section name popup
- As before, there is no way to change tableId - it is derived from a table name.
- When the section name is empty the table name is shown instead.
- White space for section name is allowed (to discuss) - so the user can just paste ' '.
- Empty name for a page is not allowed (but white space is).
- Some bugs related to deleting tables with attached summary tables (and with undoing this operation) were fixed (but not all of them yet).
Test Plan: Updated tests.
Reviewers: georgegevoian
Reviewed By: georgegevoian
Subscribers: georgegevoian
Differential Revision: https://phab.getgrist.com/D3360
Summary:
Summary columns now have their own conditional rules,
which are not shared with sister columns.
Test Plan: New test
Reviewers: alexmojaki
Reviewed By: alexmojaki
Subscribers: dsagal
Differential Revision: https://phab.getgrist.com/D3388
