Skip to content

Experimental brew load function#4069

Draft
G-Ambatte wants to merge 40 commits intonaturalcrit:masterfrom
G-Ambatte:experimentalBrewLoad
Draft

Experimental brew load function#4069
G-Ambatte wants to merge 40 commits intonaturalcrit:masterfrom
G-Ambatte:experimentalBrewLoad

Conversation

@G-Ambatte
Copy link
Copy Markdown
Collaborator

@G-Ambatte G-Ambatte commented Mar 2, 2025

This experimental PR demonstrates a process for loading brew data from homebrew.jsx rather than from app.js. This prevents user data (like </script>) from appearing in the emitted HTML source (and potentially crashing the web app).

As an example, the following is a brew that contains the title TITLE, text content of TEXT, and style content of STYLE - none of which appear in the start_app function call:

<script>start_app({"version":"3.17.0","url":"/share2/YcHkHfN17mRI","account":{"username":"sor_robertson","issued":"2025-03-01T05:27:16.385Z"},"enable_v3":true,"enable_themes":true,"config":{"local":true,"publicUrl":"https://homebrewery.naturalcrit.com","baseUrl":"http://localhost:8000","environment":"local","deployment":""},"data":{"id":"YcHkHfN17mRI","type":"share"}})</script>

If this approach was applied to all routes, it would resolve #546.

@ericscheid
Copy link
Copy Markdown
Collaborator

Also, it reduces the initial page load to just the app code, and consequently very large brews would at least present the HB UI framing earlier.

Con: it would increase the number of requests hitting the server. Not quite double, but certainly more (app+brew+css, instead of appbrew+css). Once this is working well though we could get the app.js to be cached (since it only really changes when we push code updates to prod).

@calculuschild
Copy link
Copy Markdown
Member

I agree with the idea in general. It makes sense to separate the brew contents from the appstart contents.

Given that we already have API paths for retrieving brews and things in homebrewapi.js, we should try to centralize any new brew fetching paths in there instead of in app.js.

@calculuschild calculuschild added the Approved Has been discussed and an approach is agreed upon label Mar 5, 2025
@G-Ambatte
Copy link
Copy Markdown
Collaborator Author

I've shifted the API get route to HomebrewAPI; to make it consistent with the other API routes I have added the checkClientVersion middleware, which then in turn required the API call to use the request-middleware.js function.

@calculuschild
Copy link
Copy Markdown
Member

Thinking over this some more, I have two thoughts:

  1. Should the fetching logic be put in editPage.jsx and sharePage.jsx directly? homebrewery.jsx routes us to the correct page... seems we can let those pages handle their own initial brew fetch logic upon component mount? At that point they are all client-side so it would help keep things more self-contained.

  2. We need to consider how this change will impact error handling. Current behavior will display an error popup if we are already in a page, or reroute to an error page if the error happens before the page loads (within app.js). Moving initial brew fetching to client side means we don't know if (for example) the user has authorship access until the page is already loaded, so we would need to add rerouting logic in the appropriate places.

@G-Ambatte
Copy link
Copy Markdown
Collaborator Author

1. Should the fetching logic be put in `editPage.jsx` and `sharePage.jsx` directly? `homebrewery.jsx` routes us to the correct page... seems we can let those pages handle their own initial brew fetch logic upon component mount? At that point they are all client-side so it would help keep things more self-contained.

I was initially hoping that it would be as simple as putting a "brew fetch" function in the common-to-everything React Router component, and then manipulating the data in each page as needed for that page's purpose.
However I no longer think that this approach will be viable, so my intention is to switch the fetch logic to the individual page.

2. We need to consider how this change will impact error handling. Current behavior will display an error popup if we are already in a page, or reroute to an error page if the error happens before the page loads (within `app.js`). Moving initial brew fetching to client side means we don't know if (for example) the user has authorship access until the page is already loaded, so we would need to add rerouting logic in the appropriate places.

We'll need to catch any error and throw to the error page; even SharePage should throw if the brew is locked.

@calculuschild
Copy link
Copy Markdown
Member

calculuschild commented Mar 7, 2025

We'll need to catch any error and throw to the error page; even SharePage should throw if the brew is locked.

More specifically, I'm thinking about how we will need to add some mechanism to differentiate between when to redirect to an error page vs display a popup. For example, HBError 04 occurs when the user is not signed and attempts to edit an owned brew.

If the user already had the edit page open, and somehow became signed out (cleared cookies, etc.), they will be presented with a popup when they try to save:

You are no longer signed in as an author of
this brew! Were you signed out from a different
window? Visit our log in page, then try again!

But if they are trying to open the edit page, they will instead be directed to an error page:

Sign-in required to edit this brew.

You must be logged in to one of the accounts listed as an author of this brew.
User is not logged in. Please log in here.

Brew Title: my Brew
Current Authors: calculuschild

Click here to be redirected to the brew's share page.

Both of these result from the same root error (attempt editor access of a brew without being signed in), but the result displayed depends on the error context (saving a brew you are already editing, vs opening the edit page of a brew).

If all brew retrievals now happen after the page is loaded, we lose the context. We can probably bridge this by handling the error differently if we are fetching the brew from within componentDidMount or from a user action (save), but it's something we will need to consider. Because the context matters whether the user should be allowed to stay on the page (they should get a chance to copy their work somewhere) or get shunted into an error page without seeing the content at all.

@ericscheid
Copy link
Copy Markdown
Collaborator

A side thought on routing: we don't necessarily need new/additional routes beyond /share/:id or /edit/:id if we use content negotiation. That is, a request to /edit/:id for a text/html response should return the app.js UI frame, which then issues a request for the same URI but this time for text/json.

@calculuschild calculuschild temporarily deployed to homebrewery-pr-4069 March 10, 2025 19:06 Inactive
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 March 13, 2025 01:58 Inactive
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 March 20, 2025 09:52 Inactive
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 March 22, 2025 10:29 Inactive
@G-Ambatte
Copy link
Copy Markdown
Collaborator Author

The latest commits add the ErrorPage component to the SharePage. If an error occurs while fetching the brew data, the error information is presented in the ErrorPage component instead of the normal SharePage content. As best I have been able to determine, this is identical to the current ErrorPage presented for the same error.

@G-Ambatte
Copy link
Copy Markdown
Collaborator Author

Weird issue I haven't been able to hammer out yet - the first page is always rendered using the Legacy renderer, even if the document is set to V3.

// Remove Edit ID
req.brew.editId = undefined;
// Increase view count and lastViewed property
await HomebrewModel.increaseView({ shareId: req.params.id });
Copy link
Copy Markdown
Member

@calculuschild calculuschild Apr 20, 2025

Choose a reason for hiding this comment

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

Just thinking out loud:

This no longer handles the case to increase view count for unstubbed Google-only brews. Was this intentional? I suppose this makes sense; view count/lastViewed only really matters in the context of the HB environment itself, and is not required to "rebuild" the document if HB goes down. Now that we have stubs, Google files do not need to contain their own viewing statistics.

If GoogleActions.increaseView() is no longer being used anywhere, let's remove it or put a note somewhere to remove it later.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point - I thought that this had already been changed, but on checking now I see that GoogleActions.increaseView is called in the current version of the SharePage. However, is the view count something that should be stored outside of the brew stub, now that they exist?
I'm tempted to just replicate the functionality and leave the philosophical questions for another day.

// If logged in user is not an author:
if(!req.brew.authors.includes(req?.account?.username)){
// Remove Edit ID
req.brew.editId = undefined;
Copy link
Copy Markdown
Member

@calculuschild calculuschild Apr 20, 2025

Choose a reason for hiding this comment

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

This appears to be performing part of the sanitizeBrew() function (removing the editId), but not the other parts (removing _id and __v). Do we need to handle that here by bringing over sanitizeBrew(), or is it already covered somewhere in the fetching process?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the function in the latest commit to also remove the _id and __v properties.

return res.status(200).send(brew.style);
},

getMeta : async (req, res, next)=>{
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see what getMeta is doing, but I am concerned this basically means we have to request the brew twice when opening a page (once for the metadata and again inside the sharePage.jsx).

Not sure if there is a way to apply the metadata at the same time of the initial brew fetch from the sharePage, which would at least save a Mongo call.

But at a minimum, I think we can use await api.getBrew('share', true) so we only fetch the stub.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've kept this separate at this stage because I'm not sure whether the metadata information needs to be returned in the initial response in order for the meta tags to be picked up correctly for social media previews.

I think that you are likely correct about getBrew('share', true).

@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 August 21, 2025 07:00 Inactive
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 August 21, 2025 07:37 Inactive
@5e-Cleric 5e-Cleric added the blocked Waiting on a dependency, other feature, etc., first label Nov 20, 2025
@5e-Cleric
Copy link
Copy Markdown
Member

Let's block this until the unification of edit and new page is completed: #1800

@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 January 18, 2026 02:14 Inactive
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 January 18, 2026 03:34 Inactive
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 January 18, 2026 03:47 Inactive
@G-Ambatte
Copy link
Copy Markdown
Collaborator Author

While testing this today using this 584 page brew: https://homebrewery-pr-4069.herokuapp.com/share/R9uVUjwBDxeV

  • Chrome v143 appears to render correctly
  • Firefox v147 appears to crash and never renders the brew

Further investigation will be required to determine the cause.

@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 March 29, 2026 00:37 Inactive
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 March 29, 2026 01:07 Inactive
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-4069 March 29, 2026 09:43 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved Has been discussed and an approach is agreed upon blocked Waiting on a dependency, other feature, etc., first experiment

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

Script injection vulnerability

4 participants