Refresh account on update #476

2024-02-23T08:32:25+08:00

JoshAntBrown commented

2024-02-23 08:32:25 +08:00

(Migrated from github.com)

/claim #475

Uses the new refresh feature as part of Turbo 8 to broadcast refreshes and morph page updates into the page, avoiding the need to manually refresh the page, or have to build manual updates to parts of the page.

Video attached below, I did the whole setup as demonstrated in the original issue so the main change features about half-way through.

https://github.com/maybe-finance/maybe/assets/1793797/9f324c44-dba4-4a44-bb72-c4f053a1cc14

/claim #475 Uses the new refresh feature as part of Turbo 8 to broadcast refreshes and morph page updates into the page, avoiding the need to manually refresh the page, or have to build manual updates to parts of the page. Video attached below, I did the whole setup as demonstrated in the original issue so the main change features about half-way through. https://github.com/maybe-finance/maybe/assets/1793797/9f324c44-dba4-4a44-bb72-c4f053a1cc14

🚀 3

robzolkos (Migrated from github.com) reviewed 2024-02-23 10:29:54 +08:00

app/models/account.rb

						
				@@ -1,4 +1,5 @@

				class Account < ApplicationRecord

				  broadcasts_refreshes

robzolkos (Migrated from github.com) commented

2024-02-23 10:29:54 +08:00

❤️

robzolkos (Migrated from github.com) approved these changes 2024-02-23 10:31:08 +08:00

zachgoll (Migrated from github.com) reviewed 2024-02-23 21:42:44 +08:00

zachgoll (Migrated from github.com) left a comment

Nice solution here! Just a few small questions/comments:

1. State after broadcast

Small nitpick, but any chance we could get this to reset to the default "closed" state after the refresh broadcast?

2. ActionCable Adapter

To keep things simple for local development, I was thinking we'd use the async adapter for Action Cable, but I realized that turbo-rails deliberately updates this setting to Redis. I'm still getting up to speed with the Rails/Turbo ecosystem, so wondering if there was a rationale behind this published somewhere?

Furthermore, since we're using Postgres to back GoodJob, was wondering if we could also get away with Postgres for Action Cable in prod. Curious to hear thoughts on the viability of that to avoid the Redis dep?

Nice solution here! Just a few small questions/comments: **1. State after broadcast** Small nitpick, but any chance we could get this to reset to the default "closed" state after the refresh broadcast? ![CleanShot 2024-02-23 at 08 34 58](https://github.com/maybe-finance/maybe/assets/16676157/cd8b74fb-5ae1-4149-b814-8ff0e4625a63) **2. ActionCable Adapter** To keep things simple for local development, I was thinking we'd use the `async` adapter for Action Cable, but I realized that `turbo-rails` [deliberately updates this setting](https://github.com/hotwired/turbo-rails/commit/29fe2cf14b0055804fcd426aadc7d79e67286635) to Redis. I'm still getting up to speed with the Rails/Turbo ecosystem, so wondering if there was a rationale behind this published somewhere? Furthermore, since we're using Postgres to back GoodJob, was wondering if we could also get away with Postgres for Action Cable in prod. Curious to hear thoughts on the viability of that to avoid the Redis dep?

JoshAntBrown commented

2024-02-24 00:20:56 +08:00

(Migrated from github.com)

Ah good questions!

I've solved the reset, this was happening because the turbo frame was only being cleared - it still had the src set which means when the refresh happened it triggered it to refetch the contents of the frame.

As for the adapter, there's very little documented about it. The docs simply say that the async adapter doesn't support Turbo Stream broadcasting. It doesn't say anything about the Postgres adapter though so that might be worth a try.

I'm in and out at the moment, but I'll have a play with Postgres as the adapter later today.

Ah good questions! I've solved the reset, this was happening because the turbo frame was only being cleared - it still had the src set which means when the refresh happened it triggered it to refetch the contents of the frame. As for the adapter, there's very little documented about it. The docs simply say that the async adapter doesn't support Turbo Stream broadcasting. It doesn't say anything about the Postgres adapter though so that might be worth a try. I'm in and out at the moment, but I'll have a play with Postgres as the adapter later today.

zachgoll commented

2024-02-24 00:28:14 +08:00

(Migrated from github.com)

@JoshAntBrown awesome, thanks for the fix on that!

And regarding Postgres, I'm kind of sitting in the camp of, "let's see how far we can get with it". That way, we avoid the Redis dep for now and only add it if Postgres causes significant problems. @Shpigford any preference here?

@JoshAntBrown awesome, thanks for the fix on that! And regarding Postgres, I'm kind of sitting in the camp of, "let's see how far we can get with it". That way, we avoid the Redis dep for now and only add it if Postgres causes significant problems. @Shpigford any preference here?

Shpigford commented

2024-02-24 00:44:17 +08:00

(Migrated from github.com)

@zachgoll My hunch is we're gonna end up using Redis regardless (even if just as a cache), but I'm fine with a limited bit of exploration. I don't think it's worth going to great lengths to avoid it, though.

👍 1

JoshAntBrown commented

2024-02-24 02:13:00 +08:00

(Migrated from github.com)

I've updated the adapter for development, it just uses the Active Record's connection pool so nothing else to configure. I think for caching you can probably use Solid Cache if you want to avoid Redis completely.

❤️ 1

JoshAntBrown commented

2024-02-24 04:07:10 +08:00

(Migrated from github.com)

Btw, I think we can probably further simplify how the valuation#create action works.

We could remove valuations/create.turbo_stream.erb and replace it with a simple redirect. This would help reduce a chunk of complexity since we can just have the accounts#show page render the current state and then have Turbo refresh it either as part of the redirect or by broadcasting it's changes.

The only blocker to doing that right now is how to render the 'Syncing' message. The account#show view uses @account.status == "SYNCING" to determine whether the account is currently syncing. However, this flag isn't set until the job starts and the job completes almost immediately. Therefore, the user would never see this message.

To work around this, we could update the status on the account as part of the valuation#create action. I feel like this might be the wrong approach though, and just adds complexity elsewhere.

Instead, it might be better if we had a way to query any pending or active sync jobs. This could perhaps be a status on each valuation that gets updated once the valuation has synced, or another model that tracks syncing jobs. It feels a bit beyond the scope of this PR though, especially since a refactoring of the syncing mentioned in #474, so perhaps something to consider at a later date?

Btw, I think we can probably further simplify how the `valuation#create` action works. We could remove [valuations/create.turbo_stream.erb](https://github.com/maybe-finance/maybe/blob/7e324f1b53f38f669761c9ee169224a9b21fb83a/app/views/valuations/create.turbo_stream.erb#L1-L4) and replace it with a simple redirect. This would help reduce a chunk of complexity since we can just have the `accounts#show` page render the current state and then have Turbo refresh it either as part of the redirect or by broadcasting it's changes. The only blocker to doing that right now is how to render the 'Syncing' message. The `account#show` view uses `@account.status == "SYNCING"` to determine whether the account is currently syncing. However, this flag isn't set until the job starts and the job completes almost immediately. Therefore, the user would never see this message. To work around this, we could update the status on the account as part of the `valuation#create` action. I feel like this might be the wrong approach though, and just adds complexity elsewhere. Instead, it might be better if we had a way to query any pending or active sync jobs. This could perhaps be a status on each valuation that gets updated once the valuation has synced, or another model that tracks syncing jobs. It feels a bit beyond the scope of this PR though, especially since a refactoring of the syncing mentioned in #474, so perhaps something to consider at a later date?

zachgoll commented

2024-02-24 05:50:58 +08:00

(Migrated from github.com)

@JoshAntBrown I agree that the valuation CRUD flows could be simplified. You're more than welcome to move things around if you see a clear path with that. Most of what is on main is a "sketch" that we'll need to tidy up once all the core stuff is in there, so improvements are definitely welcome!

Regarding the sync status message—I think what we have here is okay for now even though the user will likely just see a "flash" due to how fast the job is. That message is mostly a UI placeholder until the official designs are completed for how to show these "sync" states.

@JoshAntBrown I agree that the valuation CRUD flows could be simplified. You're more than welcome to move things around if you see a clear path with that. Most of what is on `main` is a "sketch" that we'll need to tidy up once all the core stuff is in there, so improvements are definitely welcome! Regarding the sync status message—I think what we have here is okay for now even though the user will likely just see a "flash" due to how fast the job is. That message is mostly a UI placeholder until the official designs are completed for how to show these "sync" states.

👍 1

JoshAntBrown commented

2024-02-24 05:56:20 +08:00

(Migrated from github.com)

Thanks, and yeah makes sense. I'll have a think about it and may follow up with a separate PR at some point if I can think of a nice way of tidying up some of the sync management. Hopefully what we have should be good enough for now.

zachgoll commented

2024-02-24 05:59:37 +08:00

(Migrated from github.com)

@JoshAntBrown sounds great! I have a PR coming shortly that adds a barebones Transaction model and updates the sync job to incorporate transactions. Once that's in, I think we'll start to get a much clearer picture of how this sync job, Account, Valuation, Transaction, and AccountBalance should all be working together.

@JoshAntBrown sounds great! I have a PR coming shortly that adds a barebones `Transaction` model and updates the sync job to incorporate transactions. Once that's in, I think we'll start to get a much clearer picture of how this sync job, `Account`, `Valuation`, `Transaction`, and `AccountBalance` should all be working together.

zachgoll (Migrated from github.com) approved these changes 2024-02-24 06:40:33 +08:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#476