rails / mission_control-jobs

Dashboard and Active Job extensions to operate and troubleshoot background jobs
MIT License
611 stars 71 forks source link

Add a max height to job arguments, fix rendering ActiveRecord keyword arguments #125

Closed ibrahima closed 1 week ago

ibrahima commented 5 months ago

When the arguments are e.g. ActiveRecord records with a lot of fields, the table gets really long and it's hard to scroll around and find things. Adding a max-height makes it easier to navigate.

Before:

Job arguments can be super long which is unhelpful when trying to navigate the table.

After:

Reasonable row height (10rem) so that you can easily scroll through and find things
20rem is okay too but still pretty tall

I also realized that the reason I have these massive job argument rows in the UI is because we are using keyword arguments with ActiveRecord models, which was incorrectly spitting out the full text representation rather than just the GlobalID as it does when using regular arguments, so I fixed that as well.


I picked a very arbitrary height, could probably make it even smaller, though I felt that being able to see a decent portion of the arguments is mildly helpful when they are long. Arguably we could make it even shorter though and you can just click through to the job details to see the full arguments.

I have no idea what if any CSS practices are used in this repo (there was hardly any CSS to reference lol) so please feel free to suggest any changes desired. Thanks!

ibrahima commented 5 months ago

Oh hm, I think my root issue is actually slightly different, so while this might still be a good idea for long simple arguments, there might be a better solution for my problem. My problem stems from the fact that our app is using GlobalID arguments with ActiveRecord models and also using keyword arguments.

It looks like in the JobsHelper module it tries to avoid deserializing GlobalID arguments, but if using keyword arguments, the first level argument will be a hash, and so it goes into the last case and deserializes the models, leading to a printout of all the record's fields. If it was kept to the globalID identifiers it wouldn't be so bad. (It is kind of handy to see the whole thing though, so I'm a little torn, but I feel like the table definitely isn't the place to see all of the record's fields.) So maybe a fix to handle that would be good...

Hm, I have a couple proposals to handle that.

The first recursively handles nested hashes:

https://github.com/rails/mission_control-jobs/compare/main...ibrahima:handle-keyword-arguments?expand=1

The second only specifically handles Ruby keyword arguments as indicated by the presence of the "_aj_ruby2_keywords" key.

https://github.com/rails/mission_control-jobs/compare/main...ibrahima:handle-aj-keyword-arguments?expand=1

Which would be better? My feeling is recursing might be necessary because if the reason to avoid deserializing GlobalID args is to avoid classes that don't exist in the app, you would need to apply this no matter how deeply nested a GlobalID argument is. However only handling keyword arguments is a narrower change.

I'll put the latter change in this PR for now because it seems more predictable in terms of unintended consequences, although I think recursing might be better. Actually, Array arguments always call as_renderable_argument on their elements so maybe that is safe to do for Hashes too... and we rely on as_renderable_argument to call ActiveJob::Arguments#deserialize for non-array/hash arguments.

ibrahima commented 2 months ago

@rosa Hi! Sorry to bother you, but I was wondering if you had a chance to look at this. Perhaps the max-height thing is more a matter of taste, but I think the issue with deserializing keyword globalID arguments is a bug. For now I've monkey patched it in my applications but I would love it if I didn't have to. Thanks!

rosa commented 3 weeks ago

Hey @ibrahima, sorry for the delay! I've been busy with other stuff and neglecting this gem, but I'm catching up now.

I think the issue with deserializing keyword globalID arguments is a bug

Yes, I agree! Good catch. Going to review this and get it merged soon.

rosa commented 1 week ago

I think to make progress here we should have a test first, with different arguments and the results we expect to see for each, instead of trying to catch those by trial and error.

Did this in #201, so I'm going to close this one. Now nested global IDs won't be deserialized, but other serialized arguments like dates will continue to be correctly deserialized.

ibrahima commented 1 week ago

Nice thanks!