Skip to content

Conversation

@wrose504
Copy link
Collaborator

Posts and comments currently show dates based on UTC, but ideally would use the browser timestamp or a user-defined setting.

This PR introduces a trait to provide access to a session variable for displayTimezone that is currently updated when the page renders, since we need client-side JavaScript to run to fetch the timezone.

In the current site this is captured for logged-in user profiles as an offset from US Pacific time, which is a little odd. I think the main issue is that we have to wait until we've rendered some content and sent it to the user for the first time before we have a chance to run the JavaScript that can check their timezone, so it's likely that the first render will be wrong.

I think we could try triggering a refresh of the page (perhaps in-place with Livewire) once we have a value present in the session, as the second render should be able to use the session-stored value.

@wrose504 wrose504 requested a review from kirkaracha June 30, 2025 05:39
@wrose504 wrose504 self-assigned this Jun 30, 2025
@wrose504 wrose504 force-pushed the session-timezone-tracking branch 2 times, most recently from 1d2c462 to 8033b3d Compare July 5, 2025 17:08
Copy link
Contributor

@official-sounding official-sounding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this might be better structured as "America/Los_Angeles for non-logged in users, use a user config item for logged-in users (and maybe capture browser timezone during sign-up process to pre-populate the preference).

Otherwise, I have some mild concerns about essentially double-rendering the user's first page load, and what impact that might have on both general user satisfaction/load performance, and PageRank on pages. You would probably want a user configuration option anyways in cases where the user's TZ is wrong on their computer for various reasons.

I am also always worried about keeping too much state around in server-side sessions for users who are not logged in. That plus the double rendering also potentially results in some server load/performance issues.

The other option would be to do this 100% in JS, which does have the benefit of potentially improving cache-ability for non-logged in users.

{
$dateQueries = [
DB::raw('DATE_FORMAT(posts.created_at, "%m-%d") as month_day'),
DB::raw("DATE_FORMAT(CONVERT_TZ(posts.created_at, 'UTC', " . DB::connection()->getPdo()->quote($this->getDisplayTimezone()) . "), '%m-%d') as month_day"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there is some input validation on this, but using this raw instead of parameterized feels like a potential SQL injection vector

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm expecting DB::connection()->getPdo()->quote to do the appropriate escaping to avoid SQL injection. It's unfortunately quite awkward to add bind parameters to queries constructed by the query builder as it only uses ? placeholders, not named ones, so then you have to figure out the right ordering of bind parameters in a way that respects the rest of the query as constructed by Eloquent. I spent a while trying to do that and gave up, hence this attempt to manage interpolating a properly escaped value 😓

@script
<script>
$js('setDisplayTimezone', () => {
const timezone = Intl.DateTimeFormat().resolvedOptions().timeZone;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any concerns with potential IANA zone mismatches between what a browser will supply and what PHP understands? there's some MDN discussion about non-canonicalization of TZ names,

also, from an infra perspective, this will probably require keeping TZDB up-to-date on the relevant App & SQL instances. Probably not a real problem, but just calling out to avoid missing these updates then getting unknown timezones.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be ok as long as we're tolerant of unknown timezones server-side and let people override from a server-supplied list in preferences. For the majority of people just browsing casually the idea was to not have times show up in a weird timezone.

@wrose504
Copy link
Collaborator Author

I wonder if this might be better structured as "America/Los_Angeles for non-logged in users, use a user config item for logged-in users (and maybe capture browser timezone during sign-up process to pre-populate the preference).

The other option would be to do this 100% in JS, which does have the benefit of potentially improving cache-ability for non-logged in users.

My initial thought was 100% JS, but then it seemed like Livewire is much more based around the idea of server-side rendering applied via JS, and its DOM morphing seemed like it could handle the first page update without forcing a visible page transition. I am not completely sold on it though - I can definitely see the argument for handling it entirely on the JS side.

Otherwise, I have some mild concerns about essentially double-rendering the user's first page load, and what impact that might have on both general user satisfaction/load performance, and PageRank on pages. You would probably want a user configuration option anyways in cases where the user's TZ is wrong on their computer for various reasons.

Agreed!

I am also always worried about keeping too much state around in server-side sessions for users who are not logged in. That plus the double rendering also potentially results in some server load/performance issues.

I suppose if we want to avoid the server state we could use a dedicated cookie? 🤔

Yeah overall I think I'm inclined to follow your suggestion and send all the timestamps in America/Los_Angeles or a logged-in user's profile timezone and then adapt it for the current browser using JS.

@wrose504 wrose504 force-pushed the session-timezone-tracking branch from 8033b3d to 8074d50 Compare July 16, 2025 02:09
@wrose504
Copy link
Collaborator Author

wrose504 commented Jul 16, 2025

Oh, wait, I remember why I ended up thinking this needed to be server-side - the server has to group the posts by day, so it needs to know when the day transitions happen, otherwise things look weird, which means it's better if the server knows the display timezone rather than trying to make it something we push to the client side. If we were rendering the page entirely client-side, a la React, I think this would be a non-issue, but given the way we render it currently, putting in those headings makes it important for the server to know the render timezone.

A way around the initial render issue might be to try to use a geolocation library based on the remote IP? That way we could try to take a guess at the right timezone and still update if the browser disagreed, but hope to avoid problems in most cases. It seems like you can use MaxMind's GeoLite database for free as long as you promise to keep it up-to-date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants