-
Notifications
You must be signed in to change notification settings - Fork 7
fix: load data on compare/share page (closes #41) #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: load data on compare/share page (closes #41) #55
Conversation
WalkthroughA shared comparison page now correctly displays model response data by referencing the appropriate Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webapp/src/app/compare/share/[id]/page.tsx (1)
43-44: Define a proper type for response objects instead ofany.The
responseAandresponseBfields are typed asany, which bypasses TypeScript's type safety. Consider defining an interface that includes at least thedatafield and any other fields returned by the API.Apply this diff to add proper typing:
+interface ModelResponse { + data: string; + // Add other fields returned by the API +} + type MatchData = { id: string; winnerId: number | null; isShareable: boolean; createdAt: Date; modelA: { id: number; name: string; provider: string; modelId: string; owner: string; host: string; elo: number | null; }; modelB: { id: number; name: string; provider: string; modelId: string; owner: string; host: string; elo: number | null; } | null; prompt: { id: string; question: string; fullPrompt: string; type: string; }; - responseA: any; - responseB: any; + responseA: ModelResponse; + responseB: ModelResponse; };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/src/app/compare/share/[id]/page.tsx(4 hunks)
🔇 Additional comments (4)
apps/webapp/src/app/compare/share/[id]/page.tsx (4)
197-199: LGTM! Good fallback logic for model labels.The badge correctly displays the model name when available and not "unknown", falling back to modelId otherwise. This provides a clear, identifiable label for each model as specified in the PR objectives.
246-248: LGTM! Consistent with Model A label logic.The Model B badge uses the same fallback pattern as Model A, ensuring consistent behavior across both comparison sides.
269-269: LGTM! Completes the response display fix.The change to use
responseB.datamirrors the Model A fix and ensures both responses display correctly.
219-219: Use.datafield to correctly access response content.The change to use
matchData.responseA.datais correct. Line 127 in the same file already uses.datafor calculations, confirming this is the proper field in thematchDatastructure.(Note: Other components in the codebase such as
response-comparison.tsxandllm-judge-scorer.tsuse.responsebecause they work with different data structures; this is expected and not an error.)
Fixes #41
Summary
The compare/share page was not showing the models and responses for shared comparison links, even though the backend API returned the data. This PR wires the existing API response into the UI so that the shared page correctly displays the prompt, models, and responses.
Changes
responseA.dataandresponseB.datafrom the API to render the two model responses on the compare/share page.modelA.name/modelB.namewhen available; if the name is"unknown", fall back to displayingmodelIdso the user can still identify the models.Testing
nameormodelId).Screenshot
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.