Skip to content

Conversation

@safiya2610
Copy link

@safiya2610 safiya2610 commented Jan 9, 2026

Summary

Improves code quality and stability without changing app behavior.

Changes

  • Split monolithic logic into routers/ and services/
  • Splitting logic into routers and services improves maintainability, testability, and separation of concerns by isolating HTTP handling from core business logic.
  • Added core authentication & protected-route tests
  • Introduced locked dependency management (requirements/lock.txt)

Why

  • Cleaner architecture and easier maintenance
  • Confidence via tests
  • Prevents version-related breakages

How to Run

py -3.11 -m venv venv
venv\Scripts\activate
pip install --upgrade pip
pip install -r requirements/lock.txt
uvicorn main:app --reload
pytest -v

Future Task

Convert auth helpers into FastAPI dependencies
Add JWT-based authentication
Introduce Pydantic schemas
CI pipeline (GitHub Actions)
Docker support

What is DONE till now

  • Core BHV Prototype (FastAPI, minimal architecture)
  • User authentication system
    • Email & password based login/signup
    • Google OAuth integration
  • Image management (locally uploading)
    • Cloudinary based img uploading
  • User narratives linked with images (Anytime update & delete)
  • Optional fuzzy color–emotion analysis module (local execution)
  • Modular architecture refactor
  • Local storage & persistence
    • Local filesystem storage for images
    • SQLite database for metadata and users
  • Basic test suite using pytest
  • Dependency version stability system
    • Supported Python versions defined (3.10–3.11)
    • Dependencies split into base.in, test.in, dev.in
    • Locked dependencies generated via pip-compile
  • CI-ready project setup

ToDo

  • JWT authentication
  • Admin dashboard
  • OAuth test mocking
  • CI pipeline
  • Docker support

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @safiya2610, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the application's foundational structure and stability. It moves away from monolithic code by clearly delineating responsibilities between routing and service layers, making the codebase easier to understand, modify, and scale. The addition of comprehensive authentication tests provides a safety net for critical user flows, while locked dependencies prevent unexpected issues arising from version discrepancies.

Highlights

  • Architectural Refactoring: The application's logic has been restructured by separating concerns into dedicated 'routers/' for API endpoints and 'services/' for business logic, enhancing maintainability and testability.
  • Core Authentication Tests: New tests have been introduced to cover core authentication flows and protected routes, improving the reliability and security of user access.
  • Dependency Stabilization: Dependency management has been improved by introducing a 'requirements/lock.txt' file, ensuring consistent and reproducible builds across environments.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request is a great step towards a cleaner architecture by splitting logic into routers and services, and stabilizing dependencies. The code is generally well-structured. However, I've found a few critical issues that need to be addressed. There's a security vulnerability in the image deletion logic that could allow unauthorized file deletion, and an incorrect use of raise RedirectResponse in a dependency that will likely cause crashes in production. I've also pointed out a security concern with storing empty passwords for OAuth users. Other comments focus on improving maintainability by reducing code duplication, adding type hints, adhering to PEP 8, and fixing some invalid HTML. Addressing these points will significantly improve the application's robustness and security.

Comment on lines +17 to +18
if not request.session.get("user"):
return RedirectResponse("/login", 303)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This authentication check is duplicated in several routes (/, /update-narrative). To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, this logic should be extracted into a shared FastAPI dependency. You've already created a require_user dependency in app/routers/gallery.py. Once corrected, it should be moved to a central location (e.g., app/dependencies.py) and reused here. The endpoint would then look like:

from app.dependencies import require_user

@router.post("/update-narrative")
def update_narrative(
    user: str = Depends(require_user),
    filename: str = Form(...),
    # ...
):
    # ... no need for the manual session check
    cur.execute(..., (..., user))
    # ...

Comment on lines +10 to +11
if not request.session.get("user"):
return RedirectResponse("/login", 303)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This authentication check is duplicated logic. To improve maintainability and follow the DRY principle, you should use a shared FastAPI dependency for authentication. The require_user dependency you've created elsewhere would be perfect for this, once it's fixed and centralized.

Comment on lines +13 to +17
try:
with open("cleaned_data.json", "r", encoding="utf-8") as f:
CLEANED = json.load(f)
except Exception as e:
CLEANED = {"colors": []}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Catching a broad Exception is generally not recommended as it can hide unexpected errors. It's better to catch more specific exceptions, such as FileNotFoundError and json.JSONDecodeError. Additionally, the caught exception e is not logged, which makes debugging difficult. Consider using the logging module to record the error.

Suggested change
try:
with open("cleaned_data.json", "r", encoding="utf-8") as f:
CLEANED = json.load(f)
except Exception as e:
CLEANED = {"colors": []}
try:
with open("cleaned_data.json", "r", encoding="utf-8") as f:
CLEANED = json.load(f)
except (FileNotFoundError, json.JSONDecodeError) as e:
# Consider logging the error, e.g., logging.warning("Could not load cleaned_data.json: %s", e)
CLEANED = {"colors": []}

bestd = d
best = name

if bestd > 45:
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The value 45 is a "magic number." It's not immediately clear what it represents. To improve readability and maintainability, it should be defined as a named constant at the top of the file, for example: MAX_COLOR_DISTANCE_THRESHOLD = 45.


pos = {"happiness", "joy", "optimism", "love", "peace", "purity", "energy", "enthusiasm"}
neg = {"sadness", "grief", "anger", "fear", "death", "disgust"}
neu = set()
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The variable neu is defined but never used. This is dead code and should be removed to improve code clarity.

class="narrative-box"
placeholder="Write a note about this image..."
onblur="this.form.submit()"
>{{ img[2] if img[2] not in ['private','public'] else '' }}</textarea>
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This logic {{ img[2] if img[2] not in ['private','public'] else '' }} is confusing. It appears to be checking the narrative field (img[2]) for visibility values. However, the database schema has distinct columns for narrative and visibility. This suggests a potential data modeling issue or a bug where data is being stored incorrectly. This code is hard to understand and maintain. Please clarify why this check is necessary or refactor the data handling to keep these concerns separate.

safiya2610 and others added 3 commits January 9, 2026 18:01
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@safiya2610
Copy link
Author

@pradeeban @mdxabu
I’ve opened this PR to improve structure, stability, and test coverage without changing existing behavior.
Please have a look whenever you get time.

@mdxabu mdxabu added the on hold Not merging this PR now. label Jan 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on hold Not merging this PR now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants