Merge Release/1.15 into master #1946
Merged
Conversation
This file contains 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
…tive (#1831) * make decorator case insensitive * release notes
In this patch we're creating some new tests to verify the expected way that the search field will filter down available tags. This is to help us make updates to the tag searching and suggesting interface. I've removed the RegExp and rewritten the filtering loop. Why? The RegExp added a step of confusion where we needed to parse and understand what it was doing. It turns out that in the course of this refactor I found a case where it wasn't doing what we thought it was doing, skipping a full-text match for `tag:` given a tag named `tag:`. Without the RegExp we simply have fewer concepts squished into this function. The filtering loop also was iterating over every tag in the account and checking if they matched the search query before it thought about pruning down the list of matches to the maximum number. With the new loop we're using a helper function (to keep the looping details out of the logic) which will abort out of its looping as soon as we find as many results as we'll allow. Otherwise if it never finds the maximum results it will continue iterating until it has scanned every tag. One final bug was fixed: it was previously possible to enter whitespace into the search field and then it would look for all of the tags as possible matches. With only whitespace in the search field there should be no tag suggestions.
There is a typo in the variable rename function that is causing tags to be set as undefined on tag rename.
Our AppVeyor builds have been failing due to an issue with `node-gyp`. In this patch we're updating the version of node run in the CI environments. This not only addresses the `node-gyp` issue but it also keeps us more up to date with node.
Tests have been failing because `noteTitleAndPreview` is reported to be taking longer than 1ms to run. In fact, `Date.now()`, which we were using to validate this, includes reduced time precision for protection against timing attacks. In this patch I've done two things to address the failing tests: - Replace `Date.now()` with `process.hrtime()` which gives us nanosecond precision - Run the function-under-test 100 times in order to eliminate variance in the measurement While fixing this test I noted a flaw in `noteTitleAndPreview` that we missed in #1647. We created the helper function `removeMarkdownFromOutput` which transforms a string based on whether or not markdown is enabled for the note. Unfortunately for us we were setting a module-global function based on the module-init-time value `isMarkdown`, which as a function is always truthy, meaning that it would return a function that alwasy removed markdown regardless of the note. In this patch I've addressed this by passing `stripMarkdown` into that function. In the PR for #1647 we discussed this parameter and removed it but we kept `removeMarkdownFromOutput` outside the scope of `noteTitleAndPreview`. We could leave out the parameter and move the function into `noteTitleAndPreview` or we could add the parameter back. For the purpose of only creating that function once I have added the parameter back in. `noteTitleAndPreview` gets called in some inner loops and I preferred to avoid recreating the filtering function on every call, or at least until the JIT figured it out; I'm sure that the performance impact is insignificant but I don't see any benefit to one way over the other and I'd rather do less work as the default.
…es (#1838) In `onNoteBeforeRemoteUpdate` we give ourselves a chance to rebase local edits on a note before remote changes are applied. Although we haven't identified any problem related to this refactor, we have been using indirect measures to check if the note we are working with is the note we think it is. That is, we were checking against `selectedNoteId` as a proxy for `note.id` when relying on `note.id` works and eliminates the possibility that `selectedNoteId` doesn't match `note.id` In this patch we're simplifying to rely solely on that direct measure and eliminting one possible bug.
In paFIJd-8t it was reported that note corruption was occurring when viewing the history of one note when it gets updated remotely. Although this fix may or may not address that problem a code audit revealed that in #1792 I removed what appeared to be an unused function when it was in fact used by the component's `onClickOutside` wrapper. The missing handler function closed out of the revision slider when clicking anywhere else. Without it, the thread handling the click crashed but since it was a callback it didn't crash the app. A message appeared in the developer console indicating that the handler was missing. Because we haven't been closing the revision slider it has become possible to start editing a note while the revisions are still open, something prevented before by way of the click handler. Thus, if viewing a revision, clicking anywhere else, and continuing, then we fill the editor with what it thinks is a note (and not a revision) and then will respond to remote updates with the contents of that revision before accepting remote updates.
Resolves #1843 Previously it has been the case that if you attempt to copy something in the note editor but there is no selection then it will wipe out the system's copy buffer. This is because we implement our own copy handler which strips away the visual formatting of elements in the note editor. 1. Copy anything in any app, suppose you copy "test" 2. Paste that "test" anywhere, it pastes 3. Click in the note editor but don't select anything. 4. "Copy" by hitting the copy shortcut or by using a menu option 5. Paste anywhere Although we'd expect that "test" is still in our copy buffer we get the empty string pasted in and "test" is gone. While not really _wrong_ or _broken_ this is a bit jarring and it's probably better in most cases to preserve the previous copy buffer if we're not copying anything. We've turned the copy command into a "clear the copy buffer" command if there's no content. In this patch we're aborting the copy if our selection is empty and that will preserve the existing buffer and the expected behavior.
Add Types to state ui actions
Introduces a basic design for managing our Redux store in the typesystem and establishes norms for how to create new action types and reducers as well as how to add types to our React components and interact with Redux
In this patch we're adopting the changes in #1855 and updating legacy reducers, action types, and action creators. This serves both as a guide for adding new Redux pieces while improving the overall type-safety and completeness of the Redux subsystem in the app.
Merge release/1.14 into develop.
* Updates Dependencies to latest * Release Note * Update deps
Removes unneeded action declarations. We intended to remove them as part of this PR: https://github.com/Automattic/simplenote-electron/pull/1856/files#diff-aef681720de17f639e0032e4e0730d05R5
When Simperium connects and disconnects we display different text and a different icon in the sync status popover. This PR moves that state from flux to the redux state.
When @belcherj asked about creating types for the existing actions in `app-state` I was reluctant since we want to get rid of them but then in considering his view I think it's the right decision until they are all gone; so I have created the types here. These are organized as one big `LegacyAction` type that isn't exported. This provides auto-completion and type-checking while still hiding the types somewhat. They are at the end of `action-types.ts` so that we can mostly ignore them. As we remove functionality from `app-state` we should remove the corresponding legacy action types.
Props to @qualitymanifest for putting this together.
This PR moves the selected note state from flux to redux. It also eliminates the need for selectedNoteId in appState. Co-authored-by: Dennis Snell <[email protected]>
In #1851 we refactored the selected note state but during testing missed a reference to `state.appState.note` which should have been updated to `state.ui.note`. Nothing crashed in the app because it was being used for its truthiness but the result was that when checking if Markdown was enabled for any given note the answer would always end up as "no." In this patch we're restoring the state reference which fixes that bug and restores the preview button in the note toolbar.
Move unsyncednoteids state from flux to redux
* Fix: Update selected note when making changes in the editor view Resolves #1879 It appears that we removed a flow in #1851 which updated app state in response to updates from the note editor[1]. While this didn't impact the operation of the editor it did present itself as a bug when previewing markdown-rendered notes. The editor is stateful on its own and so continued to function independently of the state of the selected note's content. The preview, however, grabs the contents of the selected note whenever it's activated and then renders those into a DOM node. This means that it showed the old contents of the note unless you navigated to another note and back, which swapped out the selected note with the new updated contents. In this patch, which currently is probably wrong in some way, we're updating the selected note when calling `onChangeContent` so that we don't get out of sync with the data sources. Hopefully at the end of our state cleanup and stabilization project this will disappear but for now this fix catches the gap that left split data sources. Probably there were other bugs we didn't discover as a result of this. Mainly we need to test and audit for traces of cycles/loops in the update so that we don't start spinning or overwriting data we just entered. In my brief testing the easy cases worked as I expected them to simply and singly. [1]: https://github.com/Automattic/simplenote-electron/pull/1851/files#diff-3c87750a7cc32e0127447ffc78f5c0d5L300 * Handle tag updates too by always re-selecting note on updates from the server When tags were being updated in the pap they also came through this function but they weren't coming through `onUpdateContent`, meaning that our fix overlooked tags and probably also system tags and the like. In this patch we're no longer selectively updating the note if it has a remote patch (indicating that it came from another device). Instead, we're always updating and selectively setting `hasRemoteUpdate` based on that knowledge. This ensures that we maintain consistency with local updates.
This updates the Electron dependencies to the latest. In doing so we need to have a minimum of Node 12 which is required by Electron 7. We also need to move the Windows build from AppVeyor to CircleCi.
https://electronjs.org/docs/all#appgetname is deprecated, https://electronjs.org/docs/all#appname should be used instead. This PR replaces uses of `.getName()` with `.name`.
We pass an object to a isDevConfig but it only uses the one property. This changes the function to only accept that property and types it as a boolean. I updated the one place where the function is called. I also updated the tests.
Adding types here as we continue toward a fully-typed app.
Adding types here as we continue toward a fully-typed app.
The analytics module has been using a commonJS format and has been working "by accident"
because `webpack` properly translates the import/export syntax.
```js
// moduleA.js
const stuff = {
thing: 5;
}
module.exports = stuff;
// moduleB.js
import { thing } from 'moduleA'
```
^^^ that shouldn't work but it has been. This has been resolved in this commit.
Previously the tests were a bit flakey, working some times and not others. In this patch we've added the `--runInBand` option to `jest` so that the tests run sequentially in the same process as the test runny. It may have been that `jest` was attempting to run the tests in parallel even though the nature of testing Electron/Spectron doesn't support this and so we would sometimes fail based on concurrency conflicts. A few new tests have been added in order to verify that the new updates are working and to demonstrate how we can start adding testable behaviors into the app. We still need to improve how the tests are organized and setup to minimize the noise in the code and isolate the tests. This patch fixes two bugs related to `waitForExist()` that led to the tests passing when they should have failed. The first bug was a type error whereas `waitForExist()` returns a promise-like object and `expect(...waitForExist()).toBeTruthy()` would therefore always be true. This meant that we not only didn't wait for the elements to appear but also that our check never failed. This has been fixed by awaiting the return. This has been handled inside of a new test helper function to cut down on incidental noise. The second bug was that, despite the type signaures in the webdriverio type definitions, `app.client` and `app.client.$()` return different types, whereas `app.client` returns a `Client` and `$()` returns a "WebElement JSON Object" or something that _isn't_ the same as the docs suggest. This bug was an infinite hang when trying to wait on the elements to appear since the returned value never resolved. Now all calls to wait on an element are handled by `app.client.waitForExist(selector, ...)` instead of using `$()` Further we have introduced a new helper via the `__TEST__` macro so that we can signal from inside the app specific events that we want our tests to wait for. This is performed with `waitForEvent()` * Appease the linter
* Move previous index store to Redux * Account for close notes that dont set index * Remove debugging code * Rework the previousIndex logic * Properly set selected note to null on trashing * Immediately handle trash/restore/delete operations In this change we're clearing out the selected note immediate when performing the trash/restore/delete-forever operations to avoid showing an unnecessary interstitial Simplenote logo or the newly-rendered note contents. In addition to clearing out the note state we are immediately triggering a note filter operation to update the note list and avoid a similar awkward in-between state. * Missed some obvious references Co-authored-by: Dennis Snell <[email protected]>
dmsnell
previously requested changes
Mar 9, 2020
dmsnell
dismissed
their stale review
Noted exception to merge in Slack - was performed in order to bring in a fix when the other commits in develop had no code impact since the release was cut.
dmsnell
requested changes
Mar 9, 2020
dmsnell
approved these changes
Mar 10, 2020
|
From what I can tell…
|
dmsnell
requested changes
Mar 10, 2020
dmsnell
approved these changes
Mar 10, 2020
|
|
|
Thank you @dmsnell! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PER merges the
release/1.15branch intomasterto prepare things for the release.Review
Only one developer is required to review these changes, but anyone can perform the review.
Release
These changes do not require release notes.