public api code - #60
Open
yuan-yin-truly wants to merge 1 commit into
Open
yuan-yin-truly wants to merge 1 commit into
yuan-yin-truly wants to merge 1 commit into
Conversation
yuan-yin-truly
requested review from
a team and
MattClarke131
and
a lite review from Copilot
September 13, 2026 23:05
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues remain in startup integration, endpoint behavior, validation, and the public API contract.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds the initial FastAPI server structure and versioned public API for issue #46.
Changes:
- Adds FastAPI application setup and
/api/v1routing. - Defines public search, location, and feedback schemas/routes.
- Adds internal API placeholders, startup documentation, and Python dependencies.
File summaries
| File | Description |
|---|---|
server/src/main.py |
FastAPI application entry point |
server/src/api/public/schema.py |
Public request and response models |
server/src/api/public/router.py |
Public API routes |
server/src/api/internal/schema.py |
Internal schema placeholder |
server/src/api/internal/router.py |
Internal route placeholder |
server/README.md |
Local server startup documentation |
server/pyproject.toml |
Server dependencies and metadata |
Review details
Suppressed comments (2)
server/README.md:6
- The repository's standard
npm run devstill launchesserver/src/index.tsvia the workspace script, so it never starts this FastAPI app and/api/v1/*is unavailable in the normal development workflow. Update the workspace orchestration to launch uvicorn (and resolve the two server entrypoints), or document this as a separately managed service.
# Launch Python FastAPI
cd server
uv sync
uv run uvicorn main:app --app-dir src --reload
server/src/api/public/schema.py:26
- The public search response is an unbounded list with no pagination metadata or stable location identifier, and the router defines no location-detail route. That does not establish the locations/detail and pagination conventions called for by issue #46; add an explicit paginated response and identifier/detail contract before treating
/searchas the client-facing API.
class SearchResponse(BaseModel):
locations: list[LocationResponse] = Field(
default_factory=list,
description="A list of locations matching the search criteria.",
)
- Files reviewed: 7/7 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @router.get("/search", response_model=SearchResponse) | ||
| async def search_location(search_input: Annotated[SearchInput, Depends()]): | ||
| # TODO: Implement the logic for searching items by category or query | ||
| return SearchResponse() |
Comment on lines
+21
to
+24
| @router.post("/feedback", status_code=status.HTTP_201_CREATED) | ||
| async def submit_user_feedback(feedback: FeedbackInput): | ||
| # TODO: Implement the logic for submitting user feedback | ||
| pass |
Comment on lines
+7
to
+9
| item_category: str | None = Field( | ||
| default=None, description="The item category, e.g., 'shoes', to search for." | ||
| ) # TODO: to restrict item_category to etl.dtos.ItemCategory |
Comment on lines
+15
to
+19
| class LocationResponse(BaseModel): | ||
| name: str | ||
| lat: float | ||
| lon: float | ||
| address: str # TODO: restrict to etl.dtos.Address |
| name: str | ||
| lat: float | ||
| lon: float | ||
| address: str # TODO: restrict to etl.dtos.Address |
Comment on lines
+22
to
+26
| class SearchResponse(BaseModel): | ||
| locations: list[LocationResponse] = Field( | ||
| default_factory=list, | ||
| description="A list of locations matching the search criteria.", | ||
| ) |
cnapolit
requested changes
Sep 14, 2026
Collaborator
There was a problem hiding this comment.
Looks good. After the one recommended change, adding the relevant ticket to the commit message, & me testing this out locally, it should be good to merge. Thanks @yuan-yin-truly!
|
|
||
|
|
||
| @router.get("/search", response_model=SearchResponse) | ||
| async def search_location(search_input: Annotated[SearchInput, Depends()]): |
Collaborator
There was a problem hiding this comment.
We probably don't need search_input since the idea behind the main locations endpoint was just to spit out the entire list of tuples (location, item, service) & let the frontend sort it out.
This was mostly because we expect the number of locations barely brake 1k, if it ever does.
Author
There was a problem hiding this comment.
Thanks @cnapolit! Do you mean to return (location, item, activity) tuples?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For issue #46
server/, withpublic/forclient/andinternal/foretl.etl/may need code refactoring to create a common dependency foretc/andserver/, instead of one another.