Open
Conversation
Contributor
Reviewer's GuideRefactors profile route handlers to integrate Supabase for streetpass visitor queries, visit recording and emote updates, and streamlines profile HTML/CSS persistence via Sequence Diagram: Fetching Own Profile's Visitors via SupabasesequenceDiagram
actor Client
participant App as Express App (/profile)
participant User_Model as User Model
participant Streetpass_Model as Streetpass Model
participant DB_Supabase as Supabase
Client->>+App: GET /profile (own)
App->>App: ensureAuthenticated (verifies req.user)
App->>+User_Model: findById(req.user.id)
User_Model-->>-App: User object
App->>+Streetpass_Model: getVisitors(user.id, 10)
Streetpass_Model->>+DB_Supabase: Query streetpass_visits table (SELECT ... WHERE profile_id = user.id)
DB_Supabase-->>-Streetpass_Model: Visitor data
Streetpass_Model-->>-App: Formatted visitors list
App->>Client: Render profile page with visitors
ER Diagram: User and Streetpass Visits (Supabase Table)erDiagram
User {
int id PK "User ID"
string username
string profileHtml
string profileCss
bool streetpassEnabled
}
streetpass_visits {
int id PK "Visit ID"
int visitor_id FK "Visitor (User ID)"
int profile_id FK "Visited Profile (User ID)"
datetime visited_at
string emote
}
User ||--o{ streetpass_visits : "visited_by (visitor_id)"
User ||--o{ streetpass_visits : "has_visitors (profile_id)"
Class Diagram: User and Streetpass Model ChangesclassDiagram
class User {
+id: string
+username: string
+profileHtml: string
+profileCss: string
+streetpassEnabled: boolean
+findByIdAndUpdate(id, data): Promise~User~
<<Mongoose Model>>
Note: Streetpass visitor data now handled by Streetpass model and Supabase
}
class Streetpass {
<<Model>>
+getVisitors(profileId: string, limit: int): Promise~Array~Visitor~
+recordVisit(visitorId: string, profileId: string, emote: string): Promise~void~
+updateEmote(visitId: string, emote: string): Promise~void~
Note: Interacts with Supabase 'streetpass_visits' table
}
User ..> Streetpass : Uses for streetpass logic
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
Comment on lines
18
to
28
| // User's own profile page | ||
| router.get('/', ensureAuthenticated, async (req, res) => { | ||
| try { | ||
| // Find the user with their streetpass visitors populated | ||
| const user = await User.findById(req.user._id) | ||
| .populate({ | ||
| path: 'streetpassVisitors.user', | ||
| select: 'username customGlyph avatar' | ||
| }); | ||
|
|
||
| // Get recent visitors | ||
| const visitors = user.streetpassVisitors | ||
| .sort((a, b) => b.timestamp - a.timestamp) | ||
| .slice(0, 10); | ||
| // Find the user by ID | ||
| const user = await User.findById(req.user.id); | ||
|
|
||
| // Get recent visitors from Supabase | ||
| const visitors = await Streetpass.getVisitors(user.id, 10); | ||
|
|
||
| // User's items in the Scrapyard | ||
| const userItems = await ScrapyardItem.find({ creator: user._id }) |
Contributor
There was a problem hiding this comment.
Error Handling Improvement
The asynchronous operations within the profile page route handler lack comprehensive error handling for each individual database query. This could lead to unhandled promise rejections if one of the queries fails.
Recommendation:
Wrap each asynchronous operation (User.findById, Streetpass.getVisitors, ScrapyardItem.find) in separate try-catch blocks or chain them with proper error handling to ensure that all errors are caught and handled appropriately, thus improving the robustness of the application.
Comment on lines
254
to
282
| try { | ||
| const { emote } = req.body; | ||
| const profileUser = await User.findOne({ username: req.params.username }); | ||
|
|
||
| if (!profileUser) { | ||
| return res.status(404).json({ error: 'User not found' }); | ||
| } | ||
|
|
||
| // Find the visitor entry and update the emote | ||
| const visitorIndex = profileUser.streetpassVisitors.findIndex( | ||
| v => v.user.toString() === req.user._id.toString() | ||
| ); | ||
|
|
||
| if (visitorIndex !== -1) { | ||
| profileUser.streetpassVisitors[visitorIndex].emote = emote; | ||
| await profileUser.save(); | ||
| return res.json({ success: true }); | ||
| } else { | ||
|
|
||
| // Look up the visit record in Supabase | ||
| const { data: visit, error } = await supabase | ||
| .from('streetpass_visits') | ||
| .select('id') | ||
| .eq('visitor_id', req.user.id) | ||
| .eq('profile_id', profileUser.id) | ||
| .single(); | ||
|
|
||
| if (error && error.code !== 'PGRST116') { | ||
| throw error; | ||
| } | ||
|
|
||
| if (!visit) { | ||
| return res.status(400).json({ error: 'No visit found' }); | ||
| } | ||
|
|
||
| await Streetpass.updateEmote(visit.id, emote); | ||
| return res.json({ success: true }); | ||
| } catch (err) { | ||
| console.error(err); | ||
| res.status(500).json({ error: 'Server error' }); |
Contributor
There was a problem hiding this comment.
Security and Error Handling Concerns
The route handling the update of the 'streetpass emote' lacks input validation, which could lead to security vulnerabilities such as injection attacks. Additionally, the error handling could be more descriptive to aid in debugging and user feedback.
Recommendation:
- Validate the
emoteinput to ensure it meets expected criteria (e.g., length, character type) before processing it. - Enhance error messages to provide more context about the failure, helping in quicker resolution and better user experience.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @numbpill3d - I've reviewed your changes - here's some feedback:
- Extract the duplicated visitor-fetching logic in both profile routes into a single helper in Streetpass to reduce code repetition and improve maintainability.
- Ensure all Supabase queries are properly handled with try/catch or explicit error checks to avoid unhandled rejections, ideally centralizing error handling for clarity.
- Consider using a single upsert or transaction for recording a visit and updating its emote in Supabase to guarantee atomicity and prevent race conditions between separate calls.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }); | ||
|
|
||
| await profileUser.save(); | ||
| const { data: existingVisit, error } = await supabase |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Handle and rename the Supabase error
Consider checking and logging the error value to avoid silently ignoring Supabase errors. Using .maybeSingle() can help handle 'no rows found' without throwing, and renaming error to fetchError improves clarity.
| .sort((a, b) => b.timestamp - a.timestamp) | ||
| .slice(0, 10); | ||
| // Find the user by ID | ||
| const user = await User.findById(req.user.id); |
Contributor
There was a problem hiding this comment.
nitpick: Standardize on one ID property
Choose either req.user.id or req.user._id consistently to avoid confusion.
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
1 participant
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.
Summary
findByIdAndUpdateTesting
npm test(fails: Cannot find module '../../../server/models/Item' and missing Supabase env vars)https://chatgpt.com/codex/tasks/task_e_68450e66e9b4832fb9d2cbcb719e0875
Summary by Sourcery
Refactor profile routes to use Supabase for streetpass visitor management and streamline profile content updates
Enhancements: