-
Notifications
You must be signed in to change notification settings - Fork 594
fix: bad URLs in request + some ebios UI/UX improvements #3273
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?
Conversation
📝 WalkthroughWalkthroughThe pull request encompasses UI styling refinements, component rendering updates using Anchor links, a data model field removal for risk assessments, and minor URL formatting adjustments across multiple frontend pages and utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@frontend/src/routes/`(app)/(internal)/ebios-rm/+server.ts:
- Around line 6-10: The endpoint string construction in the GET handler always
appends a "?" because url.searchParams is always truthy; change the condition to
check url.searchParams.toString() (or its length) and only append
"?"+url.searchParams.toString() when that string is non-empty so the endpoint
variable (constructed with BASE_API_URL, model.endpointUrl, and
url.searchParams) does not get an extraneous trailing question mark.
🧹 Nitpick comments (1)
frontend/src/routes/(app)/(internal)/ebios-rm/[id=uuid]/report/+page.svelte (1)
452-454: Consider renaming the filter callback parameter for clarity.The parameter
idis misleading since it's actually a feared event object, not an ID value. The comparisonid.id === fe.idreads awkwardly.♻️ Suggested improvement
{`@const` fearedEvents = reportData.feared_events.filter((fe) => - roto.feared_events.some((id) => id.id === fe.id) + roto.feared_events.some((rotoFe) => rotoFe.id === fe.id) )}
| export const GET: RequestHandler = async ({ fetch, params, url }) => { | ||
| const model = getModelInfo('ebios-rm'); | ||
| const endpoint = `${BASE_API_URL}/${model.endpointUrl}${ | ||
| const endpoint = `${BASE_API_URL}/${model.endpointUrl}/${ | ||
| url.searchParams ? '?' + url.searchParams.toString() : '' | ||
| }`; |
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.
Avoid appending an empty ? when there are no query params.
url.searchParams is always truthy, so this always adds ? even when the query is empty. Consider guarding on toString() to keep canonical URLs.
🛠️ Suggested fix
- const endpoint = `${BASE_API_URL}/${model.endpointUrl}/${
- url.searchParams ? '?' + url.searchParams.toString() : ''
- }`;
+ const query = url.searchParams.toString();
+ const endpoint = `${BASE_API_URL}/${model.endpointUrl}/${query ? `?${query}` : ''}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const GET: RequestHandler = async ({ fetch, params, url }) => { | |
| const model = getModelInfo('ebios-rm'); | |
| const endpoint = `${BASE_API_URL}/${model.endpointUrl}${ | |
| const endpoint = `${BASE_API_URL}/${model.endpointUrl}/${ | |
| url.searchParams ? '?' + url.searchParams.toString() : '' | |
| }`; | |
| export const GET: RequestHandler = async ({ fetch, params, url }) => { | |
| const model = getModelInfo('ebios-rm'); | |
| const query = url.searchParams.toString(); | |
| const endpoint = `${BASE_API_URL}/${model.endpointUrl}/${query ? `?${query}` : ''}`; |
🤖 Prompt for AI Agents
In `@frontend/src/routes/`(app)/(internal)/ebios-rm/+server.ts around lines 6 -
10, The endpoint string construction in the GET handler always appends a "?"
because url.searchParams is always truthy; change the condition to check
url.searchParams.toString() (or its length) and only append
"?"+url.searchParams.toString() when that string is non-empty so the endpoint
variable (constructed with BASE_API_URL, model.endpointUrl, and
url.searchParams) does not get an extraneous trailing question mark.
[1] Fix error 404 by requesting unexisting backend URL
/api/risk-assessments/risk_tolerance/endpoint.[2] Fix 301 redirect caused by bad URL formatting
/api/ebios-rm/studies(not having ending slash).[3] Fix some UI flaws includes:
[4] Improve ebios-rm report UX by using links instead of raw text for some object names for:
For the 4th thing, the reviewer shall decide if we generalize this pattern for the whole ebios report or not OR remove this pattern completly.
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.