search

fraction / oasis

Free, open-source, peer-to-peer social application that helps you follow friends and discover new ones on Secure Scuttlebutt (SSB).
http://oasis-demo.fraction.io
GNU Affero General Public License v3.0
287 stars 42 forks source link

Post preview with blobs and mentions #462

Closed cryptix closed 3 years ago

cryptix commented 4 years ago

This PR contains the outcome from %9NXKauuXCgJCHUaIXFGbpnXucAmvZ0QyQFJOAzURspc=.sha256: @cblgh & @cryptix's sprint to add post previews and blob uploads to the post composer. Next to those two big features @cblgh also implemented a variety of visual tweaks, including a more concise thread rendering, and we found enough time to improve the @mention suggestion feature.

As such we think it should:

Screenshots

Post preview

Screenshot_2020-10-19 Publish - Oasis

This is the preview flow we implemented. If you mention someone, and the match is ambiguous we show all the probable matches as cards, with a prepared markdown link that can be copied into the text. This is less complex and than an initial idea we had (which made use of <input type:radio> for each name).

Thread view

oasis-threads1

Posts now stick together much closer. We think it makes it clearer what is a conversation with replies compared to the latest or popular view where it's just a list of (unrelated) posts.

Miscellaneous

re oasis-desktop: Apart from one weird bug in the database server, it all went quite swell.. but that one is a bit frightning as cryptix couldn't figure out where it's comming from. The TL;dr for that one is: it randomly stopped showing names and avatar images for (some) people..

Our current bit-sized summary action plan is: make a release with just the new features (as non of that requires updating the database) and make a -beta release with most recent dependencies to test this further.

A Blobby Caveat A caveat with the current blob upload is that, after a file has been attached, the preview needs to be rendered (by clicking the Preview button) before anything is visible in the composer. This can be slightly confusing. The alternative would be to rely on the browser native file input, which is notoriously hard to style.

other nice things

cblgh commented 4 years ago

Oh yeah one comment regarding i18n: all strings have been i18n'd, but some of them use the english equivalent (and others are haphazaradly translated by yrs truly). As such, see the list below for which keys would be good to review/update.

Strings that need reviewing

Strings that require updating / reviewing:

christianbundy commented 4 years ago

Wow, thank for this PR! I'm not going to have time to review it today, but I'm excited to have these features merged.

Small note: it looks like tests are failing on Travis with "Error: 'load' gots invalid file data". Any idea what that means?

black-puppydog commented 4 years ago

Okay I read it all and I think I understand how it works. I'll try running it next. So excited! :heart_eyes:

black-puppydog commented 4 years ago

It worked! So happy!

black-puppydog commented 4 years ago

One thing that this doesn't do is guard against oversize content. I just pasted 8200 chars into the field and preview was pretending everything was normal and then BAM! It crashed when I hit publish. Might be a fringe case though, and in any case I could recover what was written by going one step back.

black-puppydog commented 4 years ago

Feedback for the mentions:

cinnamon-bun commented 4 years ago

Exif location data is stripped from blob uploads

:D 🏆 you win the extra credit trophy

cinnamon-bun commented 4 years ago

An idea for later:

Copy-pasting is hard to do on phones. The mentions workflow could do it for you:

On each suggested mentioned user, have a button which will put the link into your message for you. Like:

Who did you mean by "@cbl"?
- @cblgh          [Choose]
- @cblgh-phone    [Choose]

Who did you mean by "@cinn"?
- @cinnamon       [Choose]

Each Choose button would submit the form, edit the text on the server, and come back to the preview view.

cinnamon-bun commented 4 years ago

Some feedback, take with a grain of salt :)

I find the preview and compose area to be confusing. I keep trying to click and edit in the preview. Especially when there's a big section of mentions in between the two.

Things that could help

I also think action buttons like these are typically right-aligned

Screen Shot 2020-10-19 at 6 06 59 PM

cinnamon-bun commented 4 years ago

When using this branch, Patchwork and Oasis trigger a re-indexing whenever I close one and open the other. The re-indexing takes a long time, maybe 5 minutes.

This doesn't happen with the master branch of Oasis. I can't figure out why -- flotilla is the same version in both branches, and package-lock.json pulls in the same versions of flotilla's dependencies in both branches.

cryptix commented 4 years ago

it looks like tests are failing on Travis with "Error: 'load' gots invalid file data". Any idea what that means?

This is from the try {} catch {} of the exif removal. We probably should move it around after the mime check and just do it on images, which would result in less console.warn spewing.

cryptix commented 4 years ago

When using this branch, Patchwork and Oasis trigger a re-indexing whenever I close one and open the other.

This stuff is really scary... I honestly don’t know what to do about it. I understand flume conceptually but the implementation is just a black box... I also noted some thoughts on the flotilla update PR.

cryptix commented 4 years ago

On each suggested mentioned user, have a button which will put the link into your message for you.

Ha..! That’s also the idea I had but I couldn’t convince Alex that it would work well. One thing he brought up: imagine you want to mention two profiles with similar names (like alt accounts that just differ in suffix).

I‘m honestly glad we focused on the simpler solution because in the end time was getting a bit tight..

black-puppydog commented 4 years ago

I also want to point out that while this is not perfect, it is a huge improvement over the status-quo. So I wouldn't try to omnibus too many things into this PR.

The indexing issue is serious and I think that's the only blocking issue here. I'd leave the rest for subsequent PRs by whoever feels up for it.

sgwilym commented 4 years ago

A caveat with the current blob upload is that, after a file has been attached, the preview needs to be rendered (by clicking the Preview button) before anything is visible in the composer. This can be slightly confusing. The alternative would be to rely on the browser native file input, which is notoriously hard to style.

Just a heads up. I had the same need in react-earthstar recently, and came upon a pretty simple, non-hacky solution:

<input type="file" id="upload-input" />            
<label for={'upload-input'}>Upload blob</label>
#upload-input {
  visibility: hidden;
  position: absolute;
}

label[for="upload-input"] {
  /* Your cool button styles */
}
black-puppydog commented 4 years ago

@sgwilym label[for... I guess 😛

sgwilym commented 4 years ago

Thanks. Yeah.

On 20 Oct 2020, at 11:31, Daan Wynen notifications@github.com wrote:

@sgwilym https://github.com/sgwilym label[for... I guess 😛

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fraction/oasis/pull/462#issuecomment-712722292, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAENPI2YZYU32WDMRDUVDITSLVKHDANCNFSM4SV2X3SA.

cblgh commented 4 years ago

Just a heads up. I had the same need in react-earthstar recently, and came upon a pretty simple, non-hacky solution: [...code...]

Yeah this is pretty much exactly what we do in this patch :) The drawback is the label won't update to show "x files selected" as the native file input does, without client-side javascript.

As regards the indexing issue, I am somewhat sure I ran into that when launching Oasis in the dev env using the latest code on master, when we were first getting started. Since it seems to be blocking, and people are certain that was not an issue prior to this PR, I'll start up my laptop to double check!

Everything else (edit: excluding the broken test ofc lol) that is commented in this PR I regard as future patches from any eager contributors, inline with Oasis's values & contract ^_^

cryptix commented 4 years ago

re the indexing:

I can't figure out why -- flotilla is the same version in both branches, and package-lock.json pulls in the same versions of flotilla's dependencies in both branches.

I also tried to understand this better and AFAICT different versions were resolved when we added the exif and mime-type packages... so for instance flumeview-query was upgrade from v6.3.0 to v8. This might also explain the re-indexing(?) (I used git diff master package-lock.json but it's very annoying to consume...)

My mea-culpa basically goes like this: I hoped npm install --save piexifjs wouldn't re-resolve everything but it seems like it did.. now once I found out how to do that I will try to clean this up.

ps: in a similar vain I'm having trouble understanding why the tests don't pass on this branch vs master. Even when I revert the changes I made to test/basic.js it just doesn't exit properly and times out..? FYI The test runner dependency tap was updated from v14.10.7 to .8 - i mean, i saw broken patch releases before but... =_=

stay tuned....

cblgh commented 4 years ago

hopefully the latest commit fixes the errors resulting from the unintended update of package-lock's transitive ssb dependencies

cryptix commented 4 years ago

thanks a ton @cblgh! with 988d35a the diff against master looks like i hoped it would!

i might need to revert the changes to tests/basic.js (the t.end vs t.error) if they still timeout.

cblgh commented 4 years ago

alright so i have pretty much rectified everything i can think of to make the CI pass. if anyone else has ideas on what the issue is (the timeout?) do reach out :))

as regards the code, other than the package-lock fix, i think everything was put through its paces & stable at the onset of the PR. i fear that the changes i introduced just to please the CI / common-good might have caused issues.

khimaros commented 4 years ago

i believe i'm able to reproduce the test failure locally. it seems that 22/22 asserts are passing, but the suite never completes and tap never exits. i added a console.log() to the end of teardown, which indicates that teardown is successful.

i've modified package.json to remove the common-goood invocation for troubleshooting purposes and have been running npm run test -- --timeout=30 to reproduce. tap is not very informative about what it's doing after teardown.

i am also able to reproduce the hang by running node test/basic.js directly, which has more useful debug output. note, however, that it will not timeout.

i did notice that the tests are connecting to my real ssb-server (should this be mocked?), and if i shut the server down while the tests are hung at the end, the test throws an exception and exits (with code 7):

Exception thrown by PacketStream substream end handler Error: unexpected end of parent stream

Error: no callback provided for muxrpc close

maybe, despite app.close() completing, the server thread is hanging on join?

if i do not have an ssb-server running when i execute the tests, I do see the following console output at the end of asserts:

node_modules/ssb-db/index.js:74:          console.log("fallback to close")
aadilayub commented 4 years ago

Not sure if this is the right place to document this issue, but I was testing blob uploads just now, and kept running into an error whenever I tried to preview.

oasis

the error shown is 

TypeError: Cannot read property 'startsWith' of undefined
    at handleBlobUpload (/home/aadil/.applications/ssb-oasis/src/index.js:291:19)
    at async preparePreview (/home/aadil/.applications/ssb-oasis/src/index.js:205:11)
    at async /home/aadil/.applications/ssb-oasis/src/index.js:884:25
    at async middleware (/home/aadil/.applications/ssb-oasis/src/index.js:1047:7)
    at async /home/aadil/.applications/ssb-oasis/src/index.js:1025:5
    at async /home/aadil/.applications/ssb-oasis/src/index.js:1020:5
    at async /home/aadil/.applications/ssb-oasis/src/http.js:113:5
    at async /home/aadil/.applications/ssb-oasis/node_modules/koa-mount/index.js:52:26
khimaros commented 4 years ago

i added a debugger; in ssb-db:75 and invoked the tests with node inspect test/basic.js, after stepping in the node debugger it seems that it is trapped in an timer/async hooks loop, which does sound like a thread that won't die:

break in node_modules/async-hook-domain/index.js:66
 64 
 65   destroy (id) {
>66     const domain = domains.get(id)
 67     debug('DESTROY', id, domain && domain.ids)
 68     if (!domain)

i am not a javascript dev and relatively unfamiliar with the node ecosystem, but hopefully this sets ya'll in the right direction.

cblgh commented 4 years ago

@aadilayub which commit are you running off of? :)

cinnamon-bun commented 4 years ago

The re-indexing issue is now fixed as of commit 4d5c571. Tested by running Oasis and Patchwork 3.18.0 one at a time, back and forth.

aadilayub commented 4 years ago

@cblgh I'm running off 4d5c571df3380f7544c00af9b5411c7dfba555c2

cblgh commented 4 years ago

(tiny update: just taking a little break before i begin bashing my head against ci issues again, i hate dealing with ci :)

black-puppydog commented 4 years ago

I know next to nothing about travis, and had to look up what tap is, but other people also had problems when running tap tests against koa code, with the tests not terminating:

https://github.com/koajs/koa/issues/1224#issuecomment-406959990

cryptix commented 4 years ago

@aadilayub I just ran into the error you described (https://github.com/fraction/oasis/pull/462#issuecomment-713661182). I believe it's some kind of race and looking at the code again, I think i have a better way of doing that. There was no reason to have the hasBlob variable, really. Patch incoming.

cblgh commented 3 years ago

i'm back at it again! the plan is to spend today reconnoitering around the spooky timeouts which cause Travis to fall on his face

cblgh commented 3 years ago

👀

now to see if it works without uncommenting a particularly troublesome route, all while leaving the workaround to definitively stop the server intact in the function call that should definitively stop the server

christianbundy commented 3 years ago

Nice! :tada:

I spent a bit of time on this yesterday too -- I thought it was the setInterval(), but commenting that out didn't fix it.

Which route is troublesome? I've put in lots of work to avoid process.exit() in the past, letting Node just close on its own when all of the work is done, but I'd be okay with a quickfix here as long as there's documentation on what's broken and why we need the duct tape.

christianbundy commented 3 years ago

nvm, looks like you nailed the problem down to the summaries route? that's very surprising, that's been around for a while and hasn't been on my radar as a trouble-maker

christianbundy commented 3 years ago

Good news: I found the problem!

Bad news: The only fix I've tried is "delete the entire chunk of code". Here's a diff against e567443:

click to expand ```diff diff --git a/src/index.js b/src/index.js index 18e02ee..012f5b4 100755 --- a/src/index.js +++ b/src/index.js @@ -1059,9 +1059,6 @@ const app = http({ host, port, middleware, allowHost }); // stream closing" errors everywhere and breaks the tests. :/ app._close = () => { cooler.close(); - setTimeout(() => { - process.exit(); - }, 20000); }; module.exports = app; diff --git a/src/models.js b/src/models.js index 3e91d19..08eb994 100644 --- a/src/models.js +++ b/src/models.js @@ -195,55 +195,6 @@ module.exports = ({ cooler, isPublic }) => { }); }; - cooler.open().then((ssb) => { - console.time("about-name-warmup"); // benchmark the time it takes to stream all existing about messages - pull( - ssb.query.read({ - live: true, // keep streaming new messages as they arrive - query: [ - { - $filter: { - // all messages of type:about that have a name field that is typeof string - value: { - content: { - type: "about", - name: { $is: "string" }, - }, - }, - }, - }, - ], - }), - pull.filter((msg) => { - // backlog of data is done, only new values from now on - if (msg.sync && msg.sync === true) { - console.timeEnd("about-name-warmup"); - transposeLookupTable(); // fire once now - setInterval(transposeLookupTable, 1000 * 60); // and then every 60 seconds - return false; - } - // only pick messages about self - return msg.value.author == msg.value.content.about; - }), - pull.drain((msg) => { - const name = msg.value.content.name; - const ts = msg.value.timestamp; - const feed = msg.value.author; - - const newEntry = { name, ts }; - const currentEntry = feeds_to_name[feed]; - if (typeof currentEntry == "undefined") { - dirty = true; - feeds_to_name[feed] = newEntry; - } else if (currentEntry.ts < ts) { - // overwrite entry if it's newer - dirty = true; - feeds_to_name[feed] = newEntry; - } - }) - ); - }); - models.about = { publicWebHosting: async (feedId) => { const result = await getAbout({ diff --git a/test/basic.js b/test/basic.js index 2f0fca5..ef50631 100644 --- a/test/basic.js +++ b/test/basic.js @@ -15,7 +15,7 @@ const paths = [ "/profile/edit", "/public/latest", "/public/latest/extended", - // "/public/latest/summaries", + "/public/latest/summaries", "/public/latest/threads", "/public/latest/topics", "/public/popular/day", ```

I've tried only deleting the setInterval() and that doesn't fix it -- maybe we need to abort the stream when the app closes? I think this is our only pull-stream that exists outside of a route (?), so maybe that's the new pattern we need to develop support for.

christianbundy commented 3 years ago

Thanks for this patch, I'm super excited to have it in!

cblgh commented 3 years ago

yaaaaaaay!!!!!!!!!!!!!!!!!!!!!!