Heavenly Hostas #18

janine9vn · 2025-09-13T02:22:22Z

No description provided.

Placeholder material taken from one of PyScript's official example projects (https://pyscript.com/@examples/webgl-icosahedron/latest)

…ler to resize canvas.

…avenly-hostas into image-tool

…rawing. Made script runnable from outside /editor

…ft on erase mode prior to the event

This reverts commit 4019d1f.

I think I messed something up so I redid the changes

…hostas into dev

… and type checking. Also with helper methods to organise functionality. minor ruff ignore rules added

Upload and download images feature

… and type checking. Also with helper methods to organise functionality. minor ruff ignore rules added

@PiLogic

@PiLogic 's suggestions

…-README Update editor README.md

Add backend README

Add newline at end to fix linting issues

I fixed the wrong thing

Update README.md

Add ppt link

…af1fe47' git-subtree-dir: heavenly-hostas git-subtree-mainline: 49c1216 git-subtree-split: 9680437

janine9vn

The complete picture

Your project overall is a very ambitious one and executed fairly well. It's an interesting concept and all the different pieces of your project work quite well in concert. The code style is not quite cohesive, but we did only give you 10 days and a random team to code all this from the ground up. There are some areas of improvement, but nothing major or project-breaking. Well done!

README

The READMEs for all your project are fairly thorough. The backend one in particular is extremely detailed. Your primary project README does point to each project, but an explicit reference for which app to go to first would be appreciated.

Commits

From the sample of commits I reviewed and the commit messages together, they seem fairly good. Some of the commit messages could use improvement on what specifically the commit is doing. Otherwise the commits are incremental changes that are related and not large, monolithic updates which is good.

Code Quality

The code for the project as a whole is fairly solid. Within in each app there are improvements that could be made or minor errors. There is nothing critical or project-breaking though. Specific comments are left within this review, if you have any questions, answers, or clarifications feel free to comment! Broader project and file structure feedback is below.

The editor and gallery project may benefit from some restructuring. Particularly the editor application has a very large file that I think can be broken up into more logical pieces, such as splitting the file features and the shape features into their own files, and then importing them into the main file to be used. It'll let people get a better idea of the overall functionality available without needing to wade through the specific of how shapes are drawn or files dealt with.

The gallery project could also potentially benefit either from explicit editor #region declaration or splitting some of the room-centric features, mouse and movement controls, and other functionality into their respective files. This file is not as big, but I would take a look to see if it makes navigating the gallery project easier.

As a smaller note, I'm not certain the src/app and the __init__.py is necessary. Nothing jumped out at me immediately locking at the dockerfiles, but I could also have overlooked something.

janine9vn · 2025-09-26T20:12:05Z

heavenly-hostas/packages/backend/server/__init__.py

+from gotrue import CodeExchangeParams, SignInWithOAuthCredentials, SignInWithOAuthCredentialsOptions
+from pydantic import BaseModel
+
+from . import env, gh, pg, sb


Commenting here, but the filenames should be more descriptive and spelled out. Two letter shorthands are a bit hard to parse when it's half of the files for the backend. Especially for sb for Supabase which is not as commonly known as gh for GitHub.

I think it may have started out with sb for Supabase, because the Python module itself is called supabase, so obviously that was not an option for my utility file, I suppose I could have gone with supabase_utils.py or something, but evidently I decided for sb, may have been influenced by my shortened usage of env for the environment module and then gh and pg just followed the same pattern essentially and I knew that those two were relatively more well known abbreviations.

I guess it would have been better to name them all github_utils.py, postgres_utils.py, supabase_utils.py, but at that point you wonder if they should go into a utils package where you'd call them github.py, postgres.py, and then what? supabase.py is off the table as previously established, so what are your suggestions in that case? Perhaps, despite being moved to a utils directory, they should still maintain their _utils.py suffixes, that kinda makes sense to me at least.

janine9vn · 2025-09-27T19:17:03Z

heavenly-hostas/packages/backend/server/__init__.py

+    allow_methods=["*"],
+    allow_headers=["*"],


This is fine for development, but typically for proudction you want to restrict this to specific headers and specific methods.

That does make sense in terms of being more specific regarding security, but what real threats could it actually pose? I can imagine that say if I didn't want to receive some Authorization header from this other site for some reason, I'd disallow it, but I don't see a particular danger with allowing it, especially so as the origin in this case is trusted anyway.

Also I'm not sure we even need CORS support anyway. It was originally intended so that we could host the editor in an iframe (because it still needed a back end server since it uses NiceGUI), so that both the gallery and the editor could be under the same domain, but the editor needs to use session cookies for handling, well, user sessions. So, for certain requests, like publishing, we'd need to include those cookies in the request headers, however, the request would be coming from a different origin than where the cookies are set. I thought it could be solved with CORS, but apparently browsers just won't include cookies tied to another domain from a different domain regardless of CORS settings, so that failed spectacularly and the editor was just kept on the same domain the cookies would be set. (Obviously, we couldn't do anything that would involve interacting with the back end of GH pages)

(wait, is CORS what allows the client to send direct requests (from the browser) to a different origin? maybe we do need the CORS configuration after all, in which case I suppose a potential attack vector would be if the trusted origin got compromised and somebody could send any request on the behalf of a user without them even knowing to this other service and impersonate them, but I mean, if the trusted (in this case anyway) origin were to be compromised, we'd have some bigger issues, lol)

janine9vn · 2025-09-27T20:08:07Z

heavenly-hostas/packages/backend/server/utils.py

+def assure_get_env(var: str) -> str:
+    """Get an environment variable or raise an error if it is not set."""
+    value = os.getenv(var)
+    if value is None:
+        msg = f"Environment variable '{var}' is not set."
+        raise OSError(msg)
+    return value


This function is only used in env.py from what I can tell. I would probably just move it into that file itself, especially since the env.py is not intended to be a file the person setting it up edits.

This seems to be a relic from before the envs.py file existed and the environment variables were loaded in __init__.py and then potentially this function was also called in sb.py too? Either way, it's a bit of a legacy in that sense, it's a good refactoring point. Though I suppose it's possible that in the future the envs.py file might become a bit too small for all the environment variables and you'd want to read them in their own respective files? Not sure.

janine9vn · 2025-09-27T20:09:25Z

heavenly-hostas/packages/backend/server/env.py

+
+CLIENT_ID = utils.assure_get_env("CLIENT_ID")
+CLIENT_SECRET = utils.assure_get_env("CLIENT_SECRET")
+PRIVATE_KEY = Path("pydis-cj12-heavenly-hostas-app.private-key.pem").read_text().strip()


All other env vars throw an error if it's not present. Should this be the case for the PRIVATE_KEY as well?

Well, it will throw a FileNotFoundError if that key file does not exist

janine9vn · 2025-09-27T20:11:18Z

heavenly-hostas/packages/backend/server/gh.py

+        # Get SHA of the data branch to create a new branch off of in the fork
+        # r = await client.get(
+        #     f"https://api.github.com/repos/{env.GIT_UPSTREAM_OWNER}/{env.GIT_UPSTREAM_REPO}/git/refs/heads/{env.GIT_UPSTREAM_DATA_BRANCH}",
+        #     headers=headers,
+        # )
+        # r.raise_for_status()
+        # base_sha = r.json()["object"]["sha"]


This can probably just be removed if it's not necessary.

Yep, seems like it (I sometimes like to keep commented out code in case I want to go back to it (I suppose there's version control which should be handling this, but it's not always an option, because in this case what I think happened, was, I wrote this code, it didn't work, I commented this bit out, tried hardcoding that SHA, that worked, so I committed it (and if I had deleted this commented out bit, it would've been effectively lost))
At this stage though, sure, removing it is probably fine.

janine9vn · 2025-09-27T22:34:19Z

heavenly-hostas/packages/gallery/main.py

+
+
+def tp_to_slot(slot: int) -> None:
+    print(f"Going to image on index {slot}...")


A good avenue for future improvement is to convert these print statements into proper logging statements.

janine9vn · 2025-09-27T22:34:53Z

heavenly-hostas/packages/gallery/main.py

+async def main():
+    await load_images_from_listing()
+
+    while not SCENE.getObjectByName("room_0_0"):


Is there something special about this particular room? Could help to be explicit about it in a comment.

janine9vn · 2025-09-27T22:35:58Z

heavenly-hostas/packages/gallery/main.py

+    CAMERA.position.set(chunk_x * apothem * 2, CAMERA.position.y, chunk_z * apothem * 2)
+
+
+def url_process() -> None:


A docstring for this function would be immensely helpful since it is a function that's called relatively early and seems to do something important.

janine9vn · 2025-09-27T22:38:41Z

heavenly-hostas/packages/gallery/map_loader.py

+            return ROOM_TYPES._4, 0
+        # no exit
+        case z if not any(z):
+            assert False


I would rather raise an exception here instead of an assert. You can provide more information and in general I don't love assert usage in non-testing code since it can be disabled by running the interpreter with different settings.

Didn't know that last thing, you're totally right on that then!! 😅

janine9vn · 2025-09-27T22:39:40Z

heavenly-hostas/packages/gallery/pyproject.toml

+description = "An online Image Gallery rendered through Three.js and WebGL and programmed in PyScript."
+readme = "README.md"
+requires-python = ">=3.12"
+dependencies = []


I think you may have a few dependencies such as pyscript and pyodide.

Matiiss · 2025-10-26T01:32:23Z

As a smaller note, I'm not certain the src/app and the __init__.py is necessary.

That was motivated by a potentially outdated (or even false altogether) belief regarding the uv's workspace mechanism, so I just added it and didn't question it. It does seem it's not necessary indeed (at least I didn't run into any major issues (didn't do much testing though) when I did delete it and try some uv commands).

That said, the commit that adds this change has a very suspicious message, maybe uv didn't like something after all: heavenly-hostas-hosting/HHH@7422206

Sergio Díaz and others added 30 commits August 10, 2025 07:44

Gallery: First draft of README

902dfc1

Gallery: minimum functional PyScript setup w/ instructions

90813f6

Placeholder material taken from one of PyScript's official example projects (https://pyscript.com/@examples/webgl-icosahedron/latest)

Add basic drawing tool.

41a7ffa

Add colour select input

d9de2d0

Add line width input.

a0de0a7

Add pen and erase toggle. Does not affect canvas yet.

525644e

Added eraser feature

e096088

Add layout. Add dark mode. Fix clicking. Add resize window event hand…

7fd73e8

…ler to resize canvas.

Fix merge conflicts

cb3df18

Add loading screen

cfbc4fd

Added reset feature

d524e5c

Merge branch 'image-tool' of https://github.com/Matiiss/pydis-cj12-he…

5b12661

…avenly-hostas into image-tool

Reformat files

33a958f

Fix reset clearing line settings. Prevent non-left mouse click from d…

14c453a

…rawing. Made script runnable from outside /editor

Fix reset and resize changing the action back to draw mode even if le…

16ddf56

…ft on erase mode prior to the event

Turned colour selector into slot machine style

e01a322

Fix formatting

c53cc67

Reorganise UI layout. Fix formatting.

4019d1f

Revert "Reorganise UI layout. Fix formatting."

02ead65

This reverts commit 4019d1f.

Fix UI layout

f305f23

I think I messed something up so I redid the changes

Gallery: Refactored and simplified

4cd84da

Gallery: Refactored and simplified

6a0d8ad

Merge branch 'dev' of https://github.com/Matiiss/pydis-cj12-heavenly-…

1827277

…hostas into dev

Added pixel art feature

e979ed3

GTLF loader added, with debug tools

8942ff6

Added image download and upload

55fdc5e

Fix merge conflicts

e440dfa

added CanvasContext and CanvasSettings classes for better lsp hinting…

6497188

… and type checking. Also with helper methods to organise functionality. minor ruff ignore rules added

Merge pull request #9 from Matiiss/image-tool

d6e402c

Upload and download images feature

added CanvasContext and CanvasSettings classes for better lsp hinting…

219bf30

… and type checking. Also with helper methods to organise functionality. minor ruff ignore rules added

Matiiss and others added 28 commits August 18, 2025 19:31

move packages/tips.txt to packages/gallery/assets

455c2ee

Fix formatting

19ecf1a

Fix formatting

ed841af

added loading tips to gallery

d6d8f16

Update Python logo drawing

89c6592

Hopefully fix linting error

5230d6a

Fix keybinds

5169a2c

Merge branch 'dev'

fda7a4f

Merge remote-tracking branch 'refs/remotes/origin/main'

7f4baac

Added url param compat

7c6c05c

Finally added large blender file

8396d29

Changed debug flag. If not compliant please revert past commit

9ec84d4

Fixed mistake with async publish function

76a8b65

Update editor README.md

0e3e79f

Update README.md

4b48694

@PiLogic 's suggestions

Fix README lint issues

86f4803

Merge pull request #70 from heavenly-hostas-hosting/HiPeople21-editor…

3d81328

…-README Update editor README.md

Add a mostly thorough deploy guide for the project

f647a2b

Update backend deploy guide

4ff2f8a

more updates and clarifications for deployment

da5f2a1

Gallery: Update README.md

d31939f

Merge pull request #74 from heavenly-hostas-hosting/dev

c33f2d5

Add backend README

Update README.md

e9d38ba

Add newline at end to fix linting issues

Update README.md

5d1a2b1

I fixed the wrong thing

Merge pull request #75 from heavenly-hostas-hosting/HiPeople21-patch-1

474f00a

Update README.md

Update README.md

e15e097

Update README.md

9680437

Add ppt link

Add 'heavenly-hostas/' from commit '968043754a839ca23abc71f0c6a1a42a1…

61944ae

…af1fe47' git-subtree-dir: heavenly-hostas git-subtree-mainline: 49c1216 git-subtree-split: 9680437

janine9vn commented Sep 27, 2025

View reviewed changes



		def tp_to_slot(slot: int) -> None:
		print(f"Going to image on index {slot}...")

		CAMERA.position.set(chunk_x * apothem * 2, CAMERA.position.y, chunk_z * apothem * 2)


		def url_process() -> None:

Uh oh!

Heavenly Hostas #18

Are you sure you want to change the base?

Heavenly Hostas #18

Uh oh!

Conversation

janine9vn commented Sep 13, 2025

Uh oh!

janine9vn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

The complete picture

README

Commits

Code Quality

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matiiss commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

janine9vn left a comment •

edited

Loading

Matiiss commented Oct 26, 2025 •

edited

Loading