feat: flatmates matching/profiles enhancements and property search updates#15
Merged
Merged
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughThis PR adds the outgoing likes endpoint to retrieve profiles the current user has liked, filters discoverable profiles to exclude already-swiped users, hardens defensive parsing for computed property search fields, and adjusts database pool configuration parameters. ChangesUser Interactions and Infrastructure
Sequence DiagramsequenceDiagram
participant Client
participant API as GET /outgoing-likes
participant Service as list_outgoing_likes
participant DB as AsyncSession
Client->>API: limit, offset, auth
API->>Service: user_id, limit, offset
Service->>DB: query UserSwipe (liked)
Service->>DB: load target User + context
Service->>DB: fetch UserBlock records
Service->>Service: filter out blocked targets
Service->>API: list[dict] with peer + context
API->>Client: list[IncomingLikeSummary]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoFlatmates outgoing likes, profile filtering, and search improvements
WalkthroughsDescription• Added outgoing likes endpoint and service function for flatmates • Excluded already-swiped profiles from discoverable profiles list • Improved property search result mapping with type safety and error handling • Enhanced database pool configuration for better connection management • Fixed N+1 query and import cleanup in matching service Diagramflowchart LR
A["Flatmates Endpoint"] -->|"new route"| B["GET /outgoing-likes"]
B -->|"calls"| C["list_outgoing_likes"]
C -->|"filters"| D["UserSwipe + UserBlock"]
E["list_discoverable_profiles"] -->|"excludes"| F["Already swiped users"]
G["Property Search"] -->|"improved"| H["Result mapping & casting"]
H -->|"error handling"| I["Type safety"]
J["Database Config"] -->|"optimized"| K["Pool settings"]
File Changes1. app/api/api_v1/endpoints/flatmates.py
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/services/flatmates/matching.py (1)
320-348: ⚡ Quick winApply block exclusion in SQL before pagination.
limit/offsetis applied before blocked users are removed, so clients can get sparse pages and shifted pagination. Push block filtering into the query so pagination is based on visible rows.Proposed change
stmt = ( select(UserSwipe) .options(selectinload(UserSwipe.target_user), selectinload(UserSwipe.context_property)) + .where( + UserSwipe.user_id == user_id, + UserSwipe.target_type == SwipeTargetType.user.value, + UserSwipe.is_liked.is_(True), + UserSwipe.target_user_id.is_not(None), + ~UserSwipe.target_user_id.in_( + select(UserBlock.blocked_user_id).where(UserBlock.blocker_user_id == user_id) + ), + ~UserSwipe.target_user_id.in_( + select(UserBlock.blocker_user_id).where(UserBlock.blocked_user_id == user_id) + ), + ) - .where( - UserSwipe.user_id == user_id, - UserSwipe.target_type == SwipeTargetType.user.value, - UserSwipe.is_liked.is_(True), - UserSwipe.target_user_id.is_not(None), - ) .order_by(UserSwipe.created_at.desc()) .limit(limit) .offset(offset) ) outgoing_swipes = list((await db.execute(stmt)).scalars().all()) - - blocked_ids_stmt = select(UserBlock.blocked_user_id).where( - UserBlock.blocker_user_id == user_id, - ) - blocker_ids_stmt = select(UserBlock.blocker_user_id).where( - UserBlock.blocked_user_id == user_id, - ) - blocked_ids = set((await db.execute(blocked_ids_stmt)).scalars().all()) - blocker_ids = set((await db.execute(blocker_ids_stmt)).scalars().all()) - excluded_ids = blocked_ids | blocker_ids @@ items: list[dict[str, Any]] = [] for swipe in outgoing_swipes: - if swipe.target_user is None or swipe.target_user_id in excluded_ids: + if swipe.target_user is None: continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/flatmates/matching.py` around lines 320 - 348, The query currently paginates outgoing_swipes then filters out blocked users in Python, which breaks pagination; change the select(UserSwipe) stmt so it excludes blocked/blocker users in SQL by adding WHERE conditions that target_user_id NOT IN (select UserBlock.blocked_user_id where blocker_user_id == user_id) and target_user_id NOT IN (select UserBlock.blocker_user_id where blocked_user_id == user_id) or equivalent NOT EXISTS subqueries; keep the selectinload options and limit/offset on this filtered stmt, then remove the post-query excluded_ids filtering loop (the for loop should assume swipe.target_user is not None and target_user_id already filtered).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/services/property/search.py`:
- Around line 609-610: The code currently guards appending by truthiness (if
prop) which can allow non-Property values into the properties list and later
break PropertySchema.model_validate; change the condition to only append when
the extracted value is an instance of the Property class (use isinstance(prop,
Property)) so only concrete Property objects are appended to properties, leaving
other fallback results out.
- Around line 599-607: The Property model instance is being assigned computed
fields (distance_km, vector_distance, relevance_score) coming from row._mapping
but mypy raises attr-defined errors; update the three assignment statements that
set prop.distance_km, prop.vector_distance, and prop.relevance_score to append
the mypy suppression comment "# type: ignore[attr-defined]" to each assignment
so static checking is satisfied (these fields are validated later by
PropertySchema.model_validate(prop)); locate these assignments within the try
block that reads mapping = row._mapping and the subsequent checks for
"distance_km", "vector_distance", and "relevance_score".
---
Nitpick comments:
In `@app/services/flatmates/matching.py`:
- Around line 320-348: The query currently paginates outgoing_swipes then
filters out blocked users in Python, which breaks pagination; change the
select(UserSwipe) stmt so it excludes blocked/blocker users in SQL by adding
WHERE conditions that target_user_id NOT IN (select UserBlock.blocked_user_id
where blocker_user_id == user_id) and target_user_id NOT IN (select
UserBlock.blocker_user_id where blocked_user_id == user_id) or equivalent NOT
EXISTS subqueries; keep the selectinload options and limit/offset on this
filtered stmt, then remove the post-query excluded_ids filtering loop (the for
loop should assume swipe.target_user is not None and target_user_id already
filtered).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eaf761ec-a76c-46be-96a5-64e0fee5b4e1
📒 Files selected for processing (6)
app/api/api_v1/endpoints/flatmates.pyapp/core/config.pyapp/services/flatmates/__init__.pyapp/services/flatmates/matching.pyapp/services/flatmates/profiles.pyapp/services/property/search.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/services/flatmates/matching.py">
<violation number="1" location="app/services/flatmates/matching.py:327">
P2: Filter blocked users in the SQL query before applying `limit/offset`; post-pagination filtering causes short pages and inconsistent pagination.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
…ty, dedup mapping
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: This PR adds a new API endpoint, modifies core flatmates discovery logic to exclude swiped profiles, and tunes database pool settings—each change carries moderate risk and requires human review for correctness and unintended side effects.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/infrastructure/lifespan.py">
<violation number="1" location="app/infrastructure/lifespan.py:79">
P1: Do not swallow startup migration errors; logging and continuing can leave the app running against an outdated schema.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| await conn.execute(text(sql)) | ||
| logger.info("Startup migration applied: %s", label) | ||
| except Exception as exc: | ||
| logger.warning("Startup migration skipped (%s): %s", label, exc) |
There was a problem hiding this comment.
P1: Do not swallow startup migration errors; logging and continuing can leave the app running against an outdated schema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/infrastructure/lifespan.py, line 79:
<comment>Do not swallow startup migration errors; logging and continuing can leave the app running against an outdated schema.</comment>
<file context>
@@ -60,6 +61,24 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]:
+ await conn.execute(text(sql))
+ logger.info("Startup migration applied: %s", label)
+ except Exception as exc:
+ logger.warning("Startup migration skipped (%s): %s", label, exc)
+
+
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
Comment on lines
+64
to
+79
| async def _apply_pending_migrations() -> None: | ||
| """Run lightweight one-off DDL that cannot be applied via Supabase CLI migrations.""" | ||
| from sqlalchemy import text | ||
|
|
||
| async with engine.begin() as conn: | ||
| for label, sql in ( | ||
| ( | ||
| "image_category: add floor_plan", | ||
| "ALTER TYPE image_category ADD VALUE IF NOT EXISTS 'floor_plan'", | ||
| ), | ||
| ): | ||
| try: | ||
| await conn.execute(text(sql)) | ||
| logger.info("Startup migration applied: %s", label) | ||
| except Exception as exc: | ||
| logger.warning("Startup migration skipped (%s): %s", label, exc) |
There was a problem hiding this comment.
When conn.execute(text(sql)) raises (e.g., permission denied, PG version restriction), the exception is caught and logged as a warning. However, the PostgreSQL connection is now in an aborted-transaction state. When engine.begin()'s context manager exits normally (no exception was propagated), SQLAlchemy calls conn.commit(). PostgreSQL rejects the commit with "current transaction is aborted, commands ignored until end of transaction block," and this exception escapes _apply_pending_migrations entirely. The caller in create_lifespan catches it at the outer except Exception level and logs "Application startup failed," which silently skips _start_schedulers.
The defensive try/except creates a false sense of safety: the migration appears to be handled gracefully, but a DDL failure will still cause all schedulers to be omitted on that startup. The fix is to use engine.connect() with await conn.execution_options(isolation_level="AUTOCOMMIT") for DDL, or to wrap the entire async with engine.begin() block in a separate try/except in _apply_pending_migrations so failures stay contained.
…ring and HEAD's search.py structure
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
2 participants
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
Files Changed (6 files, +93 / -17)
Summary by cubic
Adds a
GET /flatmates/outgoing-likesAPI and improves flatmates discovery by filtering blocked and already-swiped users in SQL. Also fixes N+1 queries, hardens property search parsing, tunes DB pool settings, and runs a startup migration to addfloor_plantoimage_category.New Features
Bug Fixes
Propertyrow mapping with deduped mapping lookup and float casts for score fields; avoids text filter when semantic search is active; debug logs on cast failures.floor_plantoimage_categoryif missing.Written for commit 45700be. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Improvements
Greptile Summary
This PR pushes block filtering in
list_outgoing_likesinto the SQLWHEREclause (fixing the previously identified pagination truncation), fixes an N+1 by addingselectinloadfor related user/property data, pre-fetches the SQLAlchemy_mappingonce per row in the property search loop, and introduces_apply_pending_migrationsto add thefloor_planenum value at startup.matching.py: Block subqueries are now applied beforeLIMIT/OFFSET, resolving the page-size shrinkage reported in the prior review. Eager loading withselectinloadeliminates the N+1 on target-user and context-property relations.search.py: Single upfrontmappingfetch andisinstanceguard tighten the row-resolution logic; non-Property rows are now correctly excluded rather than appended via a bare type cast.lifespan.py: New_apply_pending_migrationsruns DDL at startup; the DDL-in-transaction concern flagged in the prior review thread remains unaddressed.Confidence Score: 4/5
The flatmates matching and property search changes are safe; the startup migration in lifespan.py carries a known risk where a DDL failure aborts the transaction and causes the commit to throw, potentially skipping scheduler startup.
The block-filter and N+1 fixes in matching.py are correct and address the prior review findings. The search.py row-mapping cleanup is a net improvement. The
_apply_pending_migrationsfunction wraps anALTER TYPE … ADD VALUEDDL statement insideengine.begin(): if the DDL raises, the caught exception leaves the connection in an aborted-transaction state, and the subsequent commit by the context manager fails — this secondary exception escapes the function and is caught by the outer startup handler, which will skip_start_schedulers.app/infrastructure/lifespan.py — the startup migration function needs attention due to the DDL-in-transaction behaviour.
Important Files Changed
_apply_pending_migrationsthat runsALTER TYPE ... ADD VALUEDDL insideengine.begin()— this statement cannot execute in a transaction block on PostgreSQL < 12, and a DDL failure leaves the transaction in an aborted state before the context-manager commit (flagged in prior review threads).list_outgoing_likescorrectly pushed to SQL WHERE clause, fixing the earlier pagination truncation issue; N+1 addressed withselectinload; already-matched likes still included in outgoing list (flagged in prior review).mappingfetch and movesproperties.appendinside theisinstance(prop, Property)guard; non-Property rows are now silently dropped without a trace log entry, making result-set size discrepancies harder to diagnose.Sequence Diagram
sequenceDiagram participant App as FastAPI App participant LP as lifespan.py participant DB as PostgreSQL participant Sched as Schedulers App->>LP: startup LP->>LP: _initialize_cache() LP->>DB: "engine.begin() -> ALTER TYPE image_category ADD VALUE IF NOT EXISTS 'floor_plan'" alt DDL succeeds DB-->>LP: OK LP->>DB: COMMIT else DDL fails DB-->>LP: Exception (transaction aborted) LP->>LP: logger.warning (exception swallowed) LP->>DB: COMMIT (fails - aborted txn) DB-->>LP: commit error propagates LP-->>App: Exception escapes _apply_pending_migrations end LP->>Sched: _start_schedulers(app)Reviews (4): Last reviewed commit: "merge: resolve conflicts with origin/mai..." | Re-trigger Greptile