Leaving a comment as I'm away from my computer today, but this can be updated now the routing includes the logType, to check for the logType prop directly. I can replace this in the next PR.

Updated this in df820d2 as I needed to fix up the expected types otherwise.

I think we can simply make the whole params as the key in the fourth position.

[ 'site', siteId, 'logs', params ]

It's because the keys are ordered hierarchically in the key array, and the params do no really have logical order within themselves.

Great suggestion, added in df820d2

nit: similarly, I think we should wrap this in an object rather than positional param so that we can't mess up.

Makes sense, added in df820d2

I'm a bit confused about this hasLogTypeChanged() logic. Will it be possible / is it necessary at all? Because when logType changes, so will the result of useQuery( siteLogsQuery( siteId, params ) );, so the siteLogs will be updated accordingly (either undefined when still loading, or the logs with the correct type).

Good point. That was copied over initially, but no it doesn't appear to be needed. Removed now.

Can we put inline this logic in the component?

Basically we want to avoid sharing hooks like this, unless absolutely necessary.

I can do, yes. Done in f0d878e. I have to wrap my head around not defaulting to separating hooks.

[nit] !! siteId will be always true.

Ah interesting, that's the existing logic. I've removed the siteId check in c61e39e. However I'll need to revisit anyway when looking at views, or when looking at who should be able to see data.

I think this type is unused? (within the scope if this PR)

That's true, I've removed it for now in df820d2

I wonder why this id is for... can we just utilize the timestamp field for that?

When mapping in siteLogsQuery the returned data now matches the ServerLog / PHPLog types due to the added id (same as what is was before).
It seems like the timestamp itself wouldn't necessarily be unique 🤔

I've had to update this now anyway with further changes -- I started by using the timestamp and another identifier and it still wasn't unique enough (though likely also thanks to the current testing environment where my dummy field data has a fixed timestamp), so there are several identifiers which help ensure the data has a unique id.

I see... we ideally should add this in the backend side, but we can do it later. 😄

Good point I added a task for it at DOTCOM-14208

I think these logic don't belong in the utils directory here. Let's move them to sites/logs/dataviews.ts instead (or sites/logs/dataviews/views.ts), along with the other dataviews-related things like fields and actions.

I think it's now becoming a pattern (in the dashboard code) that we colocate dateviews-related logic together 😄

Good point. I didn't see this as the core views functionality (not yet moved over) hence moving it to utils, but it still requires Dataviews so it should still be in a Dataviews directory. Moved in ebf0ae1

[nit[ I think we the generic types can be omitted because they can be inferred.

Good point, moved in df820d2

Be careful with this. From memory this was here to make the loading feel nicer between the different pages. But it means that we continue to show old data while we're loading the new data. It means that the query object sort of lies about the data it is showing.

Since this is something we want specifically for the logging UI, but maybe not for everyone who uses this query object, this option should perhaps be defined in the view layer instead 🤔 I'm not sure, I'm just thinking out loud.

What I'm talking about is this pattern where we use the query option object in the view, but mix in our UI specific query preferences that we don't want to affect all uses of the query e.g.

// In the view.tsx
useQuery( {
  ...siteLogsQuery( /* ... */ ),
  placeholderData: keepPreviousData,
} );

Thanks for pointing this out, that makes sense. Initial thoughts would be that it seems like the options are adding it in the view layer as you said, or allowing for another param that determines whether or not this is enabled. Seems the second would be clearer if anyone else uses the query object. What do you think?
I've moved it directly to the query call in the main component in a283414 for now though.

I hadn't thought about making it an an option to the query. I'm happy going with that too. In the HD, the pattern of not abstracting the details of the query from the the component is working pretty well so far. Generally devs working in the component code are going to end up very familiar with the details of how React Query works too, so it doesn't hurt to let the component some details about the query. But I'd also say that the patterns are far from settled 😅 This might be one of those cases which is borderline. We can try out the option approach and see how that goes?

For sure - the simplest way to do that looks like passing a boolean, but it's clearer to pass that as a value with an associated type (so we're not just passing 'true'), with keepPreviousData the only option.
Which I've done here:
Though if I refactor later and end up removing the need for keepPreviousData, at that stage it would make sense to remove it from the query as well I think.

I think it's clearer on a283414 😆 I don't think we should start this abstraction until it's absolutely necessary. This should be the responsibility of this particular Site Logs UI.

Not feeling strongly enough though so don't want to block over this 😄

😄 I will include revisiting this with a later task.

I think we can simply do

fetchSiteLogs( {
 ...params,
  siteId,
} );

Yes, good idea. Updated.

We can simply return the data as below and then we don't need to check data.data in the query layer.

const { data } = await ...

return data;

Also, it would be better to process id and total_results in the data layer

Ok, that makes sense, simplifying the query layer itself. I reworked this in f5eb778

I don't think we need this since v2 is in progress 😅

I was inspired by the top level email page in the V2 dashboard :) I also wanted to prevent confusion with some functionality not working for testers, but I don't think it's made a difference at this stage for that, so I'm happy to remove it (removed now).

It would be better to define it outside the component 🙂

Moved out now, thanks.

We have to move it to the main props of the CalloutOverlay

Ah gotcha, thanks for mentioning that! That's fixed now... I updated ids in the process as they weren't unique enough (in testing anyway - timestamps are the same in my testing environment given the dummy field data I'm using).

It would be better to define it outside the component to avoid re-creating the empty array on every render

Moved now, thanks.

We'd prefer to use const object instead of enum. See p1754323932129889-slack-C0982082MSR.

Thanks for highlighting that - I've updated it.

I think, siteId shouldn't be a field of SiteLogsParams. Because then it would be unnecessarily duplicated in siteLogsQuery() and its key array.

I'd suggest:

• delete it from this type
• modify the data layer signature to simply

function fetchSiteLogs(siteId: number, { /* rest of the params */) {

what do you think?

I see, yes. The way it's set up, I'm passing in a site id in the query call within the component within the params as well as independently, so I'd agree that this change makes sense - added.

I think we now don't need to export this type, and all the *FromEndpoint types.

Removed the export.

I see... we ideally should add this in the backend side, but we can do it later. 😄

labels need translation 🙂

This is just dummy data (as with actions and views) until I implement those properly in later tasks -- I stripped out the translations for the dummy data so as to avoid unnecessary translations.

Ah okay, thanks for the context.