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

[ibrahima]

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]

Added a class to the element to be able to target it.

[ibrahima]

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]

@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]

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]

I think this would prevent other serialized arguments from being deserialized. For example, with this change, a time argument looks like this:

[ibrahima]

Oh interesting, so there can be "hash" shaped job arguments that are actually serialized instances of an object? I didn't realize that... I will need to play around with this again to see how to differentiate these. I thought I was only affecting hash/keyword arguments here.

[ibrahima]

Ah okay right, that's why I initially proposed a narrower fix that only affects keyword arguments. https://github.com/rails/mission_control-jobs/compare/main...ibrahima:handle-aj-keyword-arguments?expand=1

But I realized now the same issue applies to hash arguments. And also, if we do call as_renderable_argument here instead of using ActiveJob::Arguments.deserialize(), then keyword arguments come out looking a little funny.

 gid://dummy/Model/1, {"options"=>{"user"=>"gid://dummy/User/1", "_aj_symbol_keys"=>"(user)"}, "_aj_ruby2_keywords"=>"(options)"} 

whereas without this change, it reformats the symbol keys nicely (albeit while deserializing globalID arguments).

 gid://dummy/Model/1, {:options=>{:user=>#<User id: 1, ...>}} 

I feel like given this, the ideal scenario for me would be to add an argument to ActiveJob::Arguments.deserialize to optionally disable globalID deserialization, because it seems like we generally do want to use the hash deserialization logic from there, but the hash deserialization also deserializes globalID arguments which we don't want in this case. The narrower change of only affecting keyword arguments does work but it makes the keyword arguments look uglier unfortunately. And I don't see a good way to solve this in MissionControl::Jobs without copying a bunch of the code from ActiveJob::Arguments here and stripping out the part that does globalID deserialization, which seems silly.

Do you think it would be worthwhile to do https://github.com/rails/mission_control-jobs/compare/main...ibrahima:handle-aj-keyword-arguments?expand=1 even though it makes the keyword arguments look worse (with all the metadata keys visible) or would it be better to solve it without that drawback?

[rosa]

Do you think it would be worthwhile to do https://github.com/rails/mission_control-jobs/compare/main...ibrahima:handle-aj-keyword-arguments?expand=1 even though it makes the keyword arguments look worse (with all the metadata keys visible) or would it be better to solve it without that drawback?

I think using keyword arguments whose values aren't global IDs is a more common case than using global IDs as values, so I wouldn't want to make that case worse to fix a less common one.

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.

[ibrahima]

That's a good idea, fair point. Lately in our codebases we started using keyword arguments for almost everything because the newer Ruby syntax makes it only an extra colon compared to regular arguments if your variable name matches. But I actually don't think it makes sense to overuse it like that anyway.

[rosa]

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]

Nice thanks!