Skip to content
This repository has been archived by the owner on Jun 18, 2019. It is now read-only.

Updated rusty secrets #16

Merged
merged 11 commits into from
May 25, 2017
Merged

Updated rusty secrets #16

merged 11 commits into from
May 25, 2017

Conversation

GabeIsman
Copy link
Contributor

This PR updates the UI to reflect the capabilities of the most recent rusty-secrets. It adds messaging for informing the user of mismatched shares. The UX of this flow could certainly use some love, but that should happen after the design update because the look of the recover interface will need to change.

This doesn't yet implement support for saving and recovering the MIME types of split files.

@conorsch
Copy link
Contributor

conorsch commented May 6, 2017

Rebasing on top of current master to include the dependency updates via #15.

@GabeIsman GabeIsman changed the base branch from update-dependencies to master May 7, 2017 17:46
@conorsch
Copy link
Contributor

conorsch commented May 8, 2017

Working well; I was able to confirm display of the Incompatible Shares message when combining shards from multiple split sessions. Agreed that further refinement is needed here, but after design assets settle down, to avoid redoing work endlessly.

@conorsch
Copy link
Contributor

conorsch commented May 8, 2017

Scratch my previous approval; it seems I mistakenly installed an older version of the deb (been reviewing PRs successively to catch up with Gabe's latest work). The changes in this PR do not work for me on Linux; starting the app simply shows a blank screen, unlike current master (e76d232), which works rather well.

Will try to reproduce the problem on Mac, since I need to test those builds anyway. In the meantime, @GabeIsman can you rebase this branch for me, on top of latest master? I rebased locally and the minor conflicts were simple to resolve (I accepted the version bumps in the react requirements from #15), but perhaps you'll catch something I didn't.

@conorsch
Copy link
Contributor

conorsch commented May 8, 2017

Tested Mac builds on this branch, and ran into problems there, too. Without rebasing, meaning using this feature branch as-is, nom run test fails on Mac.

Tried building anyway and installing the artifact; the build finishes, but when I try to split a secret, I can't get passed the page where I declare the secret—clicking Continue is simply a no-op (as far as I can tell).

Stepping back from review on this for now, and will move on to #17. @GabeIsman take a look when you fancy, see if you can find what I'm blind to.

@conorsch conorsch self-requested a review May 8, 2017 19:33
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Tests are failing on Mac, and .dmg doesn't work well. Should be rebased on top of latest develop and revised so the tests pass.

This was referenced May 8, 2017
@GabeIsman
Copy link
Contributor Author

@conorsch Sorry the trouble you ran into! I think this very likely caused because the native bindings for rusty-secrets haven't been rebuilt to match the new electron version or the bindings for the new version were never compiled at all. Try running npm run postinstall (this runs after npm install, but only if the latter completes without error). That should recompile the app bindings. If that doesn't work try blowing away both node_modules and app/node_modules and doing a clean npm install. npm run dev will show the inspector and any exceptions that are getting thrown if you're still having trouble.

@GabeIsman
Copy link
Contributor Author

I should also say that I confirmed that tests are passing and the app is working on my machine.

@conorsch
Copy link
Contributor

conorsch commented May 9, 2017

Ahh, of course! On Linux I've been using make clean-build which nukes the node_modules, but I did not perform the same manual action via make clean on Mac. Thanks, will do so and report back!

@conorsch
Copy link
Contributor

conorsch commented May 9, 2017

Magnificent: with @GabeIsman's counsel, was able to fix the build problems that slipped in as a result of successive repeated builds with variable dependencies. (Reckon it was npm rebuild that saved me.) Specifically, I can now confirm:

Mac:

  • npm run test passes
  • npm run test-e2e passes
  • npm run dist succeeds
  • ✅ manually installed built package behaves well

Linux:

  • npm run test passes
  • 🔴 npm run test-e2e fails (expected; we use a headless environment in linux for now)
  • 🔴 npm run dist fails (due to missing icons; which have moved around a lot due to rebasing)
  • 🔴 can't install package because package can't be built

Given the amount of outstanding changes in PRs #16 and #17, I'm going to merge here, even given he Linux problems, since otherwise I'm rebasing endlessly and testing against branches that aren't exactly what's open on GitHub. So, onward!

It's worth noting that I had to run npm rebuild after npm install completed successfully on both Mac and Linux in order for npm run test to pass. This is true of both platforms even though I'd run rm -rf node_modules/ prior to npm install. Mentioning it here in case I need to circle back.

Rather than storing the state. This is much better since with this
approach removing the mismatched share correctly updates everything
that shares are now matched.
This is an unfortunate consequence of the current two-package.json
setup that used to be required by electron-builder. Apparently recent
versions are much smarter and do not require this setup. Would be great
to get away from and merge our two package.jsons to avoid this and
other complications.
@conorsch
Copy link
Contributor

conorsch commented May 9, 2017

Rebased on top of latest master to resolve conflicts.

@conorsch
Copy link
Contributor

Still encountering severe regressions under Linux, to the point where launching the freshly built app renders only a blank window. Will carefully step through the clean-and-rebuild logic to see if I can resolve without any changes to the declared dependencies.

@conorsch
Copy link
Contributor

Unfortunately the Linux build woes remain:

sad-sunder-blank-window-on-linux

The new functionality presented here does indeed work on Mac, so I'm going to merge and open up a more narrowly scoped issue to target the Linux regressions.

@conorsch conorsch merged commit 4224cbf into master May 25, 2017
@conorsch conorsch mentioned this pull request May 25, 2017
conorsch pushed a commit that referenced this pull request Jun 9, 2017
Remerging the 'visual-update' branch due to errors in original merge in
PR #17. These must have been related to reconciliation of changes
between #16 and #17 by yours truly.
This was referenced Jun 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants