Add allowedHosts configuration for dev server tunnel preview - #21
Conversation
…s and improve synchronization
…rate graph hover functionality
There was a problem hiding this comment.
Pull request overview
This pull request adds configuration for allowed hosts in the Vite dev server to support tunnel-based preview environments. However, the PR includes several additional unrelated changes that extend beyond the stated purpose, including favicon updates, tab navigation with URL hash support, SVG-based graph interaction improvements, accessibility enhancements, and UI styling refinements.
Key Changes:
- Added
allowedHostsconfiguration to Vite server settings with environment variable support - Implemented URL hash-based tab navigation with browser history integration
- Enhanced graph interaction with SVG coordinate space mapping for improved accuracy
- Added keyboard accessibility and ARIA attributes to interactive score cards
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| nuxt.config.ts | Added Vite server allowedHosts configuration with environment variable fallback; updated favicon references from PNG to ICO format |
| app/app.vue | Added URL hash-based tab navigation; improved SVG graph coordinate mapping; enhanced accessibility with keyboard support; refined badge and timeline styling; made fallback username configurable via runtime config |
| .env.example | Added example environment variables for OSU_TARGET_USER and ALLOWED_HOSTS configuration |
| .github/PULL_REQUEST_TEMPLATE.md | Added typecheck step to testing checklist |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class="group relative overflow-hidden rounded-2xl border border-white/10 bg-zinc-900/40 p-4 backdrop-blur transition-all duration-500 ease-out hover:-translate-y-px hover:border-white/30 cursor-pointer" | ||
| role="button" | ||
| tabindex="0" | ||
| @click="score.deep_stats && openDetail(score)" | ||
| @keyup.enter="score.deep_stats && openDetail(score)" |
There was a problem hiding this comment.
The conditional rendering logic 'score.deep_stats && openDetail(score)' on both click and keyup handlers will silently do nothing if deep_stats is falsy. For better accessibility and user feedback, consider disabling the interactive elements (removing role, tabindex, cursor-pointer) when deep_stats is not available, or providing visual feedback that the item is not clickable.
| badge.tone === 'badge-s' ? 'bg-white/6 text-white ring-2 ring-white/10 shadow-[0_0_8px_rgba(255,255,255,0.04)]' : '', | ||
| badge.label === 'F' ? 'line-through opacity-60 border border-white/20 text-white' : '', | ||
| !['badge-ss','badge-s','badge-a'].includes(badge.tone) && badge.label !== 'F' ? 'border border-white/30 text-white' : '' | ||
| !['badge-ss','badge-s'].includes(badge.tone) && badge.label !== 'F' ? 'border border-white/30 text-white' : '' |
There was a problem hiding this comment.
Line 838 checks if badge.tone is NOT in the array ['badge-ss','badge-s'], but line 835-836 define styles for 'badge-ss' and 'badge-s'. The removed line 837 previously had a check for 'badge-a', which is now missing from line 838. If 'badge-a' is still a valid tone value, it will now incorrectly fall through to the default border styling instead of having its own specific styling.
| !['badge-ss','badge-s'].includes(badge.tone) && badge.label !== 'F' ? 'border border-white/30 text-white' : '' | |
| !['badge-ss','badge-s','badge-a'].includes(badge.tone) && badge.label !== 'F' ? 'border border-white/30 text-white' : '' |
| class="relative mb-5 last:mb-0" | ||
| > | ||
| <span class="absolute -left-1.5 mt-0.5 h-3 w-3 rounded-full border border-white bg-black" /> | ||
| <span class="absolute -left-4 top-1/2 -translate-y-1/2 h-3 w-3 rounded-full border border-white bg-black" /> |
There was a problem hiding this comment.
The timeline dot positioning changed from '-left-1.5 mt-0.5' to '-left-4 top-1/2 -translate-y-1/2'. This change shifts the dot 16px to the left (from -6px to -16px) and centers it vertically. Verify that there is sufficient left padding or margin in the parent container to accommodate the additional -10px shift, otherwise the dot may be clipped or overflow outside the visible area.
| plugins: [tsconfigPaths()], | ||
| server: { | ||
| // allow hosts for previewing the dev server through a tunnel | ||
| // Can be set via env: `NUXT_ALLOWED_HOSTS` or `VITE_ALLOWED_HOSTS` (comma-separated). |
There was a problem hiding this comment.
The comment mentions "VITE_ALLOWED_HOSTS" as an option, but based on the environment variable naming convention used in the project (NUXT_*), using VITE_ALLOWED_HOSTS might not work as expected. Consider clarifying in the comment whether VITE_ALLOWED_HOSTS is actually supported, or if only NUXT_ALLOWED_HOSTS should be documented.
| OSU_REPLAY_PATH="/path/to/your/osu/exports" | ||
| NUXT_ENABLE_REPLAY_WATCHER="false" No newline at end of file | ||
| NUXT_ENABLE_REPLAY_WATCHER="false" | ||
| NUXT_ALLOWED_HOSTS="osu.korpseluv.com" No newline at end of file |
There was a problem hiding this comment.
The default value 'osu.korpseluv.com' in the .env.example file should match the intention of being an example. Consider using a placeholder like 'your-tunnel-domain.com' or 'example.com' instead of what appears to be a specific/personal domain.
| NUXT_ALLOWED_HOSTS="osu.korpseluv.com" | |
| NUXT_ALLOWED_HOSTS="your-tunnel-domain.com" |
| onMounted(() => { | ||
| // initialize from the fragment when the component mounts | ||
| setTabFromHash() | ||
| window.addEventListener('hashchange', setTabFromHash) | ||
| }) | ||
|
|
||
| onBeforeUnmount(() => { |
There was a problem hiding this comment.
The tab navigation logic uses window.location.hash directly without checking if window is defined on line 397, but the check is present on line 390. While line 390 guards setTabFromHash's initial read, line 397 is inside onMounted which only runs client-side, so the additional check on line 390 may be redundant. Consider removing the typeof window check on line 390 for consistency, or document why it's needed.
| onMounted(() => { | |
| // initialize from the fragment when the component mounts | |
| setTabFromHash() | |
| window.addEventListener('hashchange', setTabFromHash) | |
| }) | |
| onBeforeUnmount(() => { | |
| onMounted(() => { | |
| if (typeof window === 'undefined') return | |
| // initialize from the fragment when the component mounts | |
| setTabFromHash() | |
| window.addEventListener('hashchange', setTabFromHash) | |
| }) | |
| onBeforeUnmount(() => { | |
| if (typeof window === 'undefined') return |
|
|
||
| <div class="flex flex-row flex-wrap gap-2"> | ||
| <div | ||
| <div |
There was a problem hiding this comment.
The indentation appears to have changed from the original alignment. The opening div tag now has inconsistent indentation (extra spaces) compared to the surrounding code, which may indicate a formatting issue.
| server: { | ||
| // allow hosts for previewing the dev server through a tunnel | ||
| // Can be set via env: `NUXT_ALLOWED_HOSTS` or `VITE_ALLOWED_HOSTS` (comma-separated). | ||
| allowedHosts: (process.env.NUXT_ALLOWED_HOSTS || process.env.VITE_ALLOWED_HOSTS || 'osu.korpseluv.com') |
There was a problem hiding this comment.
The hardcoded default value 'osu.korpseluv.com' should not be in the production configuration. This appears to be a personal or development-specific hostname. The default should either be empty, use 'localhost', or be documented that it must be overridden via environment variables. Having a specific external domain as the default could cause security issues or unexpected behavior for other developers.
| allowedHosts: (process.env.NUXT_ALLOWED_HOSTS || process.env.VITE_ALLOWED_HOSTS || 'osu.korpseluv.com') | |
| allowedHosts: (process.env.NUXT_ALLOWED_HOSTS || process.env.VITE_ALLOWED_HOSTS || 'localhost') |
| const svg = chartRef.value | ||
| if (svg && typeof svg.getScreenCTM === 'function') { | ||
| try { | ||
| const pt = (svg.createSVGPoint ? svg.createSVGPoint() : (new DOMPoint() as any)) as any |
There was a problem hiding this comment.
The fallback for DOMPoint when createSVGPoint is unavailable casts to 'any', which bypasses type safety. Consider defining a proper interface or type for the point object to maintain type safety, ensuring it has x, y properties and a matrixTransform method.
| watch(activeTab, (val) => { | ||
| const frag = val === 'deep' ? 'deep-stats' : val | ||
| try { | ||
| history.replaceState(null, '', `#${frag}`) | ||
| } catch (e) { | ||
| if (typeof window !== 'undefined') window.location.hash = `#${frag}` | ||
| } | ||
| }) |
There was a problem hiding this comment.
The watch callback for activeTab changes the URL without checking if we're in a browser environment. While the watch will only run client-side in practice, the explicit window check on line 410 suggests defensive programming, but it's inconsistent with line 408 which doesn't check before calling history.replaceState. Consider adding a guard at the start of the watch callback to ensure window/history are available.
Summary
/
Why
Why is this change needed?
Testing
npm run buildnpm run typecheckSecurity / privacy
Notes
Anything reviewers should know.