Stabilize calculations, reconcile docs, and optimize Three.js#9
Merged
Stabilize calculations, reconcile docs, and optimize Three.js#9
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR adds timezone awareness to calculation actions, standardizes planet identifiers to PlanetId, prunes and reconciles large documentation sets, optimizes ACG line scoring with latitude-bound precomputation, implements frontend lazy-loading and rollup chunking for globe code, improves heatmap/shader colorspace handling, and adds a docs-checking script. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/calculations/geospatial/search.ts`:
- Around line 449-464: The precomputeACGLatitudeBounds function currently
produces minLatitude=Infinity and maxLatitude=-Infinity for ACGLine entries with
empty points, which creates inverted bounds and causes the candidate filter
(used in the grid filtering logic) to silently exclude valid latitudes; change
precomputeACGLatitudeBounds to skip/filter out any ACGLine where line.points is
empty (e.g., if (line.points.length === 0) continue or build the map and then
.filter(Boolean)) so only lines with at least one point produce an
ACGLatitudeBound and are returned; keep the function name
precomputeACGLatitudeBounds and the ACGLine/ACGLatitudeBound types the same so
downstream code reading these bounds doesn't need changes.
In `@docs/implementation-plan/phase-6-ui.md`:
- Line 18: Change the phrase "calculator enhanced output" to
"calculator-enhanced output" in the document so the compound adjective correctly
hyphenates before "output"; search for the exact string "calculator enhanced
output" and replace it with "calculator-enhanced output".
In `@scripts/check-doc-paths.ts`:
- Line 6: The use of shell commands via execSync ('ls -1' used to build
repoTopLevel and 'rg' later in the file) is platform-dependent and can fail on
non-Unix systems; replace these with Node APIs instead: replace the execSync('ls
-1', ...) that creates repoTopLevel with a cross-platform directory read using
fs.readdirSync(process.cwd()) (or fs.promises.readdir with recursion) and build
the Set from that array, and replace the ripgrep execSync call with a recursive
directory walk (using fs and path) or a cross-platform glob library (e.g.,
fast-glob or glob) to collect matching files; update any code using repoTopLevel
and the rg results to accept arrays/Sets from these Node-based methods (look for
symbols repoTopLevel and the execSync('rg ...') invocation to locate the spots
to change).
In `@src/components/globe/shaders/loadEarthTextures.ts`:
- Around line 69-84: When a promise in results resolves, the pre-created
fallback texture in textures[key] must be disposed before assigning the loaded
texture to avoid leaking GPU resources; update the success branch (where you
check result.status === 'fulfilled') to call textures[key].dispose() (or safely
dispose if not null) before setting textures[key] = result.value, using the
existing TEXTURE_ENTRIES loop, textures object, and createFallbackTexture
pattern so both success and failure paths consistently release the initial
fallback.
In `@vite.config.ts`:
- Around line 20-27: The manualChunks function's three-vendor condition
redundantly checks both id.includes('/node_modules/three/') and
id.includes('/node_modules/three/examples/'); remove the latter redundant
includes check and keep only id.includes('/node_modules/three/') in the
manualChunks callback so files under three (including examples) are grouped into
'three-vendor' while leaving the globe chunk and default return unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/globe/shaders/loadEarthTextures.ts`:
- Around line 82-86: The failure branch is redundantly disposing and recreating
a fallback texture even though a correct fallback was already created earlier
for this key; in the else branch that handles load failure (referencing
textures[key], TEXTURE_ENTRIES[i].path and createFallbackTexture), remove the
textures[key].dispose() and the textures[key] = createFallbackTexture(...)
assignment so the preexisting fallback texture is retained instead of being
recreated.
In `@vite.config.ts`:
- Around line 17-31: The manualChunks function uses the raw id string for path
checks which should be normalized; update the manualChunks callback in
vite.config.ts to call Vite's normalizePath(id) (or import { normalizePath }
from 'vite') and use the normalized value for the includes checks (e.g., replace
id.includes('/node_modules/three/') and id.includes('/src/components/globe/')
with normalizedId.includes(...)) so path separators are consistent across
platforms and future Rollup changes.
---
Duplicate comments:
In `@scripts/check-doc-paths.ts`:
- Around line 1-13: No changes required: the cross-platform use of readdirSync
and the SKIP_DIRECTORIES Set are correct in this file; leave the repoRoot,
repoTopLevel, and SKIP_DIRECTORIES definitions (repoRoot, repoTopLevel,
SKIP_DIRECTORIES) as-is and no further fixes are necessary.
Comment on lines
82
to
86
| } else { | ||
| console.warn(`Failed to load texture "${key}" (${TEXTURE_ENTRIES[i].path}):`, result.reason) | ||
| textures[key].dispose() | ||
| textures[key] = createFallbackTexture(colorSpace, maxAnisotropy) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant dispose/recreate on failure path.
The fallback texture for this key already exists (created at lines 69–74) with the correct colorSpace. Disposing and recreating an identical fallback is wasteful—just keep the existing one.
Proposed fix
} else {
console.warn(`Failed to load texture "${key}" (${TEXTURE_ENTRIES[i].path}):`, result.reason)
- textures[key].dispose()
- textures[key] = createFallbackTexture(colorSpace, maxAnisotropy)
+ // Keep the pre-created fallback; no action needed
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/globe/shaders/loadEarthTextures.ts` around lines 82 - 86, The
failure branch is redundantly disposing and recreating a fallback texture even
though a correct fallback was already created earlier for this key; in the else
branch that handles load failure (referencing textures[key],
TEXTURE_ENTRIES[i].path and createFallbackTexture), remove the
textures[key].dispose() and the textures[key] = createFallbackTexture(...)
assignment so the preexisting fallback texture is retained instead of being
recreated.
Comment on lines
+17
to
+31
| build: { | ||
| rollupOptions: { | ||
| output: { | ||
| manualChunks: (id) => { | ||
| if (id.includes('/node_modules/three/')) { | ||
| return 'three-vendor' | ||
| } | ||
| if (id.includes('/src/components/globe/')) { | ||
| return 'globe' | ||
| } | ||
| return undefined | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
Vite manualChunks id path normalization Windows
💡 Result:
On Windows, the id you get in build.rollupOptions.output.manualChunks(id) (and in Vite plugin hooks like resolveId/load/transform) is typically already normalized by Vite to use POSIX separators (/), while Node’s path.* (and plain Windows paths) use \. So comparisons like id.includes('\\node_modules\\') or id.startsWith(path.resolve(...)) can fail on Windows. (vite.dev)
Fix: normalize before comparing (recommended)
Use Vite’s normalizePath() and compare against normalized strings:
// vite.config.ts
import { defineConfig, normalizePath } from 'vite'
import path from 'node:path'
const src = normalizePath(path.resolve(__dirname, 'src')) // "C:/.../src" on Windows
export default defineConfig({
build: {
rollupOptions: {
output: {
manualChunks(id) {
id = normalizePath(id)
if (id.includes('/node_modules/')) return 'vendor'
if (id.startsWith(src + '/')) return 'app'
}
}
}
}
})Vite documents this explicitly: normalize paths to POSIX before doing “resolved id” comparisons, and provides normalizePath() for that purpose. (vite.dev)
Quick alternative (less robust)
Just write your checks using /:
manualChunks(id) {
if (id.includes('/node_modules/')) return 'vendor'
}This works because Vite normalizes resolved ids to / even on Windows. (vite.dev)
Citations:
- 1: https://vite.dev/guide/api-plugin/?utm_source=openai
- 2: https://vite.dev/guide/api-plugin/?utm_source=openai
- 3: https://vite.dev/guide/api-plugin/?utm_source=openai
🌐 Web query:
Vite normalizePath rollup manualChunks documentation
💡 Result:
-
Vite
normalizePath(what/where): Vite exportsnormalizePathto convert Windows\separators to POSIX/so plugin/path comparisons against resolved ids behave consistently across OSes (especially when using Rollup-style include/exclude filtering). See Vite Plugin API → Path Normalization. [1]import { normalizePath } from 'vite' normalizePath('foo\\bar') // 'foo/bar'
-
Rollup
output.manualChunks(what/where): Rollup’soutput.manualChunkslets you control code-splitting either via:- object form
{ chunkName: [moduleIds...] } - function form
(id, { getModuleInfo, getModuleIds }) => chunkName | void
See Rollup Configuration Options →output.manualChunks. [2]
// function form example manualChunks(id) { if (id.includes('node_modules')) return 'vendor' }
- object form
-
Vite note about
manualChunksgoing forward: In newer Vite docs (Rolldown transition), the object form is removed and the function form is deprecated in favor of Rolldown’s code-splitting options (e.g.codeSplitting/advancedChunks, depending on the doc section/version). See Vite’s migration / Rolldown integration docs. [3]
Sources: [1] [2] [3]
🌐 Web query:
Vite rollup module id path separator behavior
💡 Result:
-
What you’ll see in
id(Rollup/Vite plugin hooks):- Vite normalizes resolved filesystem ids to POSIX separators (
/), keeping the Windows drive/volume prefix intact. Rollup (by default) keeps the platform path “as-is”, so on Windows it can be\-separated. Therefore, code that compares/matchesidcan behave differently between Rollup-on-Windows and Vite. [1]
- Vite normalizes resolved filesystem ids to POSIX separators (
-
What to do in plugins (recommended):
-
Normalize before comparing:
import { normalizePath } from 'vite' // or from '@rollup/pluginutils' if (normalizePath(id).includes('/node_modules/')) { /* ... */ }
Vite exports
normalizePath, and@rollup/pluginutilsalso provides it (it converts\→/). [1][2] -
When using RegExp matching, make separators OS-agnostic:
- Use
[\\/]instead of/in patterns that may run under Rollup on Windows, e.g./node_modules[\\/]react/. [3]
- Use
-
-
Related Rollup output behavior (chunk ids):
- Rollup fixed Windows-related slash issues so that when
preserveModulesis used, chunk ids use forward slashes even on Windows (Rollup v3.2.4). [4]
- Rollup fixed Windows-related slash issues so that when
Sources: Vite Plugin API (Path Normalization) [1], @rollup/pluginutils docs (normalizePath) [2], separator-safe regex guidance [3], Rollup release note mentioning forward slashes in chunk ids with preserveModules [4].
Use normalizePath() to align with Vite's best practices for path handling.
While Vite automatically normalizes the id parameter to POSIX separators (/) even on Windows, using normalizePath() explicitly is the documented best practice and future-proofs the code against Rollup behavior differences.
♻️ Suggested refactor
-import { defineConfig } from 'vite'
+import { defineConfig, normalizePath } from 'vite'
@@
- manualChunks: (id) => {
- if (id.includes('/node_modules/three/')) {
+ manualChunks: (id) => {
+ const normalizedId = normalizePath(id)
+ if (normalizedId.includes('/node_modules/three/')) {
return 'three-vendor'
}
- if (id.includes('/src/components/globe/')) {
+ if (normalizedId.includes('/src/components/globe/')) {
return 'globe'
}
return undefined
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vite.config.ts` around lines 17 - 31, The manualChunks function uses the raw
id string for path checks which should be normalized; update the manualChunks
callback in vite.config.ts to call Vite's normalizePath(id) (or import {
normalizePath } from 'vite') and use the normalized value for the includes
checks (e.g., replace id.includes('/node_modules/three/') and
id.includes('/src/components/globe/') with normalizedId.includes(...)) so path
separators are consistent across platforms and future Rollup changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
docs:checkwithscripts/check-doc-paths.tsto fail on broken markdown file referencesValidation
bun run typecheckbun run testbun run buildbun run docs:checkNotes
GlobeCanvasmixed static/dynamic import warningSummary by CodeRabbit
New Features
Performance
Bug Fixes
Documentation