migrate remaining dependent mongoose models to TypeScript (#7.23)#316
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. |
Om7035
left a comment
There was a problem hiding this comment.
Thanks for the migration work here — there’s a lot of good progress, and I appreciate the effort to improve typing consistency across models and tests. I’m requesting changes before merge to reduce production risk: the PR scope is currently very broad (multiple model domains and related updates in one bundle), Frontend CI is still failing (including the useEffect dependency issue around dataKey), and most of the new/updated model tests appear heavily mock-based rather than validating real schema/index/query behavior against a test DB. Could we please (1) get CI fully green, (2) add at least a minimal integration test layer (e.g., mongodb-memory-server) for key model statics/validation/index paths, and (3) harden ObjectId handling in model statics (validate/cast input IDs before querying)? Once these are addressed, I’m happy to re-review quickly — this is close, but I’d like one more pass to make it safer for production.
Om7035
left a comment
There was a problem hiding this comment.
Request changes before merge.
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.
Focused addendum (requested): PR #316 merge readiness review
Date of focused pass: 2026-04-23 (UTC)
PR: #316
Verdict
Not good to merge yet (Request changes should remain active).
Why this should not merge yet
- Open requested-changes review exists (Apr 4, 2026).
- Reviewer explicitly requested changes and has not approved yet.
- Known CI quality concerns are unresolved in the discussion context.
- Review comment cites frontend CI failures (including
useEffectdependency arounddataKey). - Merge should be blocked until all required checks are green on the latest head commit.
- PR scope is too broad for safe final review.
- This branch bundles 19 commits spanning multiple migration phases and domains, not just the “remaining dependent models.”
- Large bundled scope increases regression risk and hides root-cause attribution when failures appear.
- Test strategy appears overly mock-driven for schema migration risk profile.
- Discussion notes model tests are mostly mocked and do not sufficiently validate real schema/index/query behavior.
- For Mongoose migration PRs, a minimal integration test layer is needed before merge confidence is high.
Merge criteria I would require
- Resolve all requested-changes review feedback and obtain approval.
- Achieve fully green required checks on current HEAD.
- Add/expand integration tests (e.g., mongodb-memory-server) for key model statics/validation/index paths.
- Harden ObjectId handling in statics (explicit validate/cast before query).
- (Preferred) reduce/partition PR scope or provide a clear “stacked PR supersedes X/Y/Z” merge note.
Bottom line
At current state, do not merge PR #316. It is close, but it still needs one stabilization pass for CI and production-safety validation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This PR completes the TypeScript migration for the last remaining Mongoose models:
Vote, Reaction, Message, MessageRoom, Presence, Roster, Typing, and UserReport.
Key Updates
TypeScript Conversion
All remaining
.jsmodel files have been converted to.ts.Mongoose 8.x Compatibility
Updated schema options and methods to ensure compatibility with Mongoose v8.
Strong Typing
Added proper TypeScript interfaces for all migrated models in
app/types/mongoose.ts.Full Test Coverage
Added mocked CRUD unit tests for all migrated models.
How to Test
pnpm test