fix(torii-sql): sql playground for slot #2779
Conversation
WalkthroughOhayo, sensei! This pull request introduces changes to the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/server/static/sql-playground.html(2 hunks)
🔇 Additional comments (1)
crates/torii/server/static/sql-playground.html (1)
377-377: Verify the URL construction in different deployment scenarios.
The URL construction changes might behave differently based on:
- Base path of the application
- Proxy configurations
- Different deployment environments
Also applies to: 511-511
✅ Verification successful
URL construction is safe and self-contained, sensei!
The URL construction in sql-playground.html is using window.location.href consistently and correctly. The implementation:
- Uses relative URL construction that automatically inherits the current page's base path
- Is used consistently in both fetch calls (lines 377 and 511)
- Doesn't rely on any hard-coded paths that could break in different environments
- Automatically adapts to whatever base path or proxy configuration is in place
The search results show no other SQL endpoint consumers that would need similar updates, and the URL construction pattern is deployment-environment agnostic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other fetch calls in the codebase that might need similar updates
rg -l 'fetch\(' | xargs rg '/sql\?'
# Look for any deployment configuration files that might affect the base path
fd -t f "nginx\.conf|\.htaccess|vercel\.json|netlify\.toml"
Length of output: 99
Script:
#!/bin/bash
# Let's check for any URL construction patterns in the SQL playground HTML file
rg -A 2 "window\.location" crates/torii/server/static/sql-playground.html
# Check for any base path or URL configurations in the codebase
rg -i "base.*url|base.*path" --type rust
# Look for any fetch calls or API endpoint configurations
rg "fetch\(|/sql" crates/torii/server/static/sql-playground.html
# Check for any deployment or server configuration files
fd -t f "config|settings|deploy" -e toml -e json
Length of output: 6431
| @@ -508,7 +508,7 @@ | |||
| const startTime = performance.now(); | |||
| try { | |||
| const response = await fetch( | |||
| "/sql?" + new URLSearchParams({ query }) | |||
| `${window.location.href}?` + new URLSearchParams({ query }) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Similar URL construction improvement needed here, sensei!
For consistency and robustness, apply the same URL construction pattern here.
Here's the suggested improvement:
- `${window.location.href}?` + new URLSearchParams({ query })
+ `${new URL('sql', window.location.href)}?${new URLSearchParams({ 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.
| `${window.location.href}?` + new URLSearchParams({ query }) | |
| `${new URL('sql', window.location.href)}?${new URLSearchParams({ query })}` |
| @@ -374,7 +374,7 @@ | |||
| ORDER BY m.name, p.cid; | |||
| `; | |||
| const response = await fetch( | |||
| "/sql?" + new URLSearchParams({ query: schemaQuery }) | |||
| `${window.location.href}?` + new URLSearchParams({ query: schemaQuery }) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider improving URL construction.
The URL construction could be made more robust by handling trailing slashes and using proper URL joining.
Here's a suggested improvement:
- `${window.location.href}?` + new URLSearchParams({ query: schemaQuery })
+ `${new URL('sql', window.location.href)}?${new URLSearchParams({ query: schemaQuery })}`This ensures:
- Proper URL path joining
- Handles trailing slashes correctly
- Maintains the intended endpoint path
📝 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.
| `${window.location.href}?` + new URLSearchParams({ query: schemaQuery }) | |
| `${new URL('sql', window.location.href)}?${new URLSearchParams({ query: schemaQuery })}` |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2779 +/- ##
==========================================
- Coverage 56.01% 56.01% -0.01%
==========================================
Files 434 434
Lines 55068 55068
==========================================
- Hits 30849 30847 -2
- Misses 24219 24221 +2 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Bug Fixes