7.22 Migrate QuoteModel to TypeScript#315
Merged
Om7035 merged 2 commits intoQuoteVote:mainfrom Apr 24, 2026
Merged
Conversation
|
@shubham-01-star is attempting to deploy a commit to the Louis Girifalco's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Om7035
requested changes
Apr 4, 2026
Collaborator
Om7035
left a comment
There was a problem hiding this comment.
🚨 PR Review — Request Changes ❌
🔴 Critical Issues
1. PR scope is far too broad for the title
- PR claims “Migrate QuoteModel to TypeScript”, but includes:
- 14 commits
- Multiple models/tests
- Stripe + type-related changes
- ❗ High merge risk and difficult to review safely
2. Quote statics are weakly validated
findByPostId(postId: string)directly queries{ postId }- ❌ No ObjectId validation/casting
⚠️ Can lead to:- Inconsistent behavior
- Hidden malformed input bugs
3. Missing schema constraints for quote range integrity
startWordIndex/endWordIndexare optional but:- ❌ No non-negative validation
- ❌ No
startWordIndex <= endWordIndexcheck - ❌ No enforcement of pair presence
➡️ Risk: corrupt quote spans
4. Tests are shallow (mock-based, not real DB validation)
- Current tests:
- Mock
find().sort().limit()
- Mock
- ❌ Do NOT validate:
- MongoDB behavior
- Indexes
- ObjectId coercion
5. CI is currently failing
- ❌ 1 error + warnings
- Includes:
- Missing hook dependency (
dataKey) - Lint issues
- Missing hook dependency (
- 🚫 Blocks production-safe merge
🛠 Inline Suggestions
📁 quotevote-backend/app/data/models/Quote.ts
- Add ObjectId validation in
findByPostId - Add index range validation
- Normalize
quote(trim + bounds) - Add index:
{ postId: 1, created: 1 }
Collaborator
|
@shubham-01-star can you connect with me on discord I have sent a message to you |
Om7035
requested changes
Apr 23, 2026
Collaborator
There was a problem hiding this comment.
Primary blockers
--
- PR scope does not match title (“Migrate QuoteModel to TypeScript”).
- PR contains 14 commits and includes earlier migrations (simple models, User, Post, Comment, Stripe typing updates), not only Quote.
- This makes review and rollback high-risk.
- Recommendation: split to a Quote-only PR (or explicitly retitle/reframe as aggregate migration PR).
- Quote static method input validation gap (
findByPostId(postId: string)).
- Current approach directly queries by string input without explicit ObjectId validation/casting.
- Risk: malformed input behavior becomes inconsistent across call paths.
- Recommendation: validate/cast at static boundary and define deterministic failure behavior.
- Missing Quote span integrity constraints.
startWordIndex/endWordIndexare optional but there is no visible guard for:- non-negative values,
startWordIndex <= endWordIndex,- pair consistency (if one is set, the other must be set).
- Recommendation: add schema validators or service-level invariant checks.
- Tests are mostly mocked and do not prove database-level behavior.
- Current tests emphasize mocked chains and unit behavior.
- Missing integration checks for ObjectId casting, index behavior, and schema validator enforcement.
- Recommendation: add integration tests using in-memory Mongo for Quote model invariants.
- CI signal not clean at review time.
- Review thread cites failing checks and lint/hook issues.
- Recommendation: block merge until all required checks pass on latest head commit.
Additional recommendation
- If Quote model is merged independently, add or verify a practical read index for feed usage, e.g.
{ postId: 1, created: -1 }, based on real query plans.
Suggested merge gate for #315
- Narrow PR scope to Quote-only (or retitle as aggregate migration and split follow-ups).
- Add ObjectId validation/casting in Quote statics.
- Add quote range integrity validators.
- Add integration tests for schema + index behavior.
- Ensure required CI checks are green on head.
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.
Summary
Migrates the QuoteModel from JavaScript to TypeScript with full Mongoose 8.x typing. Adds a new
Quote.tsmodel using existingQuoteDocumentandQuoteModelinterfaces fromapp/types/mongoose.ts, and includes unit tests for schema validation and static methods.Changes
New Files
app/data/models/Quote.ts— Typed Mongoose model with:userId,postId,quote,startWordIndex,endWordIndex,createdquotefieldfindByPostId,findLatest__tests__/unit/models/Quote.test.ts— Unit tests covering:userId,postId,quote)created)startWordIndex,endWordIndex)findByPostId,findLatest)Modified Files
app/data/models/index.ts— AddedQuotebarrel exportVerification
npx tsc --noEmit— clean (exit 0)pnpm build— success (exit 0)