PL-292 Vite SSR#1360
Conversation
d13ca4d to
1715c1b
Compare
4ca45c4 to
ba8142e
Compare
49c609c to
2e2a87b
Compare
bdcfe53 to
139b483
Compare
6a9ff77 to
d03127b
Compare
64e4fa8 to
121b255
Compare
8fdf03c to
61dcade
Compare
72666cb to
f3f01e2
Compare
fa1813a to
3d2a727
Compare
3d2a727 to
5e6bfb8
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (52)
💤 Files with no reviewable changes (7)
👮 Files not reviewed due to content moderation or server errors (45)
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/MapView/MapView.js (1)
9-10:⚠️ Potential issue | 🟠 MajorReact-Leaflet still runs in SSR; the current Leaflet-loading guard doesn’t prevent server failures.
src/views/MapView/MapView.jsstatically importsreact-leaflet(import * as ReactLeaflet...anduseMapEvents) on lines 9-10, and the map subtree also contains staticreact-leafletimports (e.g.src/views/MapView/components/UserMarker/UserMarker.js). Deferring only Leaflet/plugin loading on lines 63-68 doesn’t stop server module evaluation; React-Leaflet/Leaflet touchwindow/DOM at load time and SSR must be disabled (wrap the whole map subtree in a client-only boundary or load it via dynamic import withssr: false). (react-leaflet.js.org)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/views/MapView/MapView.js` around lines 9 - 10, MapView currently statically imports react-leaflet (useMapEvents and the ReactLeaflet namespace) and related components like UserMarker, causing SSR to evaluate Leaflet code and crash; change this by removing those top-level imports and instead load the entire map subtree on the client only — either dynamically import the MapView map subtree (and components such as UserMarker) with SSR disabled (e.g., next/dynamic({ ssr: false })) or render the map subtree only after a client mount check (useEffect/setState) so useMapEvents and ReactLeaflet are only imported/executed in the browser; ensure all references to useMapEvents, ReactLeaflet, and UserMarker are moved into the client-only module or the dynamically imported component.
🧹 Nitpick comments (1)
package.json (1)
72-72: ⚡ Quick winAlign
lint:fixextensions withlint.
linthandles.mjs, butlint:fixcurrently does not. Keep them consistent to avoid missed autofixes.Suggested patch
- "lint:fix": "eslint src/ server/ server.mjs --ext .js,.jsx --fix && prettier src/ server/ server.mjs --write", + "lint:fix": "eslint src/ server/ server.mjs --ext .js,.jsx,.mjs --fix && prettier src/ server/ server.mjs --write",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 72, The lint:fix npm script is missing the .mjs extension so autofixes can miss files; update the "lint:fix" script in package.json (the lint:fix entry) to include --ext .js,.jsx,.mjs for eslint and ensure prettier invocation also targets .mjs (make the extensions consistent with the "lint" script) so .mjs files are linted and auto-fixed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Around line 6-11: The Dockerfile omits server.mjs from the build context so
the later production stage copy of /app/server.mjs into appbase/staticbuilder
fails; update the initial COPY steps to include server.mjs (the file named
server.mjs) into the image root (alongside the existing COPY of ./server and
./src) so that the downstream stage can successfully COPY /app/server.mjs —
ensure the same ownership flag (--chown=default:root) is used and that any
build-stage directory referenced by the production stage (appbase/staticbuilder)
will contain server.mjs.
In `@docs/ViteSSRMigrationPlan.md`:
- Around line 15-51: The fenced code blocks in docs/ViteSSRMigrationPlan.md are
missing language identifiers (e.g., the ASCII-art blocks and other snippets),
which triggers markdown lint MD040; update each triple-backtick fence used for
non-syntax blocks to include a language tag such as text (or bash where
appropriate). Locate the fenced blocks that contain the ASCII diagrams and
phase/service lists (the blocks shown in the diff) and change ``` to ```text (or
```bash for shell examples) so the renderer/linter recognizes them; ensure all
similar fenced blocks in the file (the ones showing diagrams and lists) are
updated consistently.
In `@scripts/update-runtime-env.js`:
- Line 23: The script always writes runtime overrides to the hardcoded outputDir
('public') so production (which serves dist/client) never gets the updated
env-config.js; change the outputDir assignment in scripts/update-runtime-env.js
(the outputDir constant) to resolve to 'dist/client' when running in production
(e.g. process.env.NODE_ENV === 'production') or, more robustly, detect and
prefer an existing 'dist/client' directory over 'public' before writing; ensure
the rest of the script still writes the env-config.js into the chosen outputDir.
In `@server.mjs`:
- Around line 203-210: The async route handlers that call await getEntry()
(e.g., the sitemap handler registered via app.use('/sitemap.xml', ...), the
robots handler app.get('/robots.txt', ...), and the other async app.get/app.use
handlers that also call getEntry()) must be wrapped so promise rejections are
forwarded to Express error middleware; wrap each async handler with the existing
asyncHandler helper (or implement a small wrapper that catches errors and calls
next(err)) so any rejection from getEntry() is passed to next rather than
causing an unhandled promise rejection.
In `@src/entry-client.jsx`:
- Around line 60-63: The current code replaces server-provided settings by
assigning preloadedState.settings = settings; instead of merging; change it so
localStorage settings overlay the server-preloaded settings by merging objects
(preserve existing preloadedState.settings keys and only overwrite with values
from SettingsUtility.getSettingsFromLocalStorage()), updating the assignment
that references SettingsUtility.getSettingsFromLocalStorage() and
preloadedState.settings accordingly.
- Around line 38-40: The tracePropagationTargets fallback produces [''] when
unset, causing Sentry to match all URLs; fix by normalizing and filtering the
value before passing it to Sentry: read config.sentryTracePropagationTargets,
split on ',', trim each entry, remove empty strings, and pass that filtered
array to the tracePropagationTargets option (the code around
tracePropagationTargets in src/entry-client.jsx). Ensure the transformation is
applied where tracePropagationTargets is constructed so untrimmed or empty
values are excluded.
In `@src/views/MapView/utils/mapActions.js`:
- Around line 9-11: panViewToBounds currently uses globalThis.L (and thus
depends on Leaflet being global) even though mapActions.js already loads Leaflet
into the module-scoped L; update panViewToBounds to use the module-scoped L
(e.g., replace globalThis.L.latLng / globalThis.L.latLngBounds uses with
L.latLng / L.latLngBounds) and add a guard to ensure L is non-null before
calling its methods so it works consistently without relying on globalThis.L or
side effects from MapView.js (which only sets globalThis.rL).
In `@vite.config.js`:
- Around line 9-42: The Vite config is using the wrong parameter name: change
the defineConfig callback parameter from ssrBuild to isSsrBuild and update the
conditional spread that currently uses !ssrBuild to use !isSsrBuild so
sentryVitePlugin is only added for non-SSR builds; update references inside the
defineConfig callback (e.g., the ternary that adds sentryVitePlugin) and ensure
the symbol names defineConfig and sentryVitePlugin remain unchanged.
---
Outside diff comments:
In `@src/views/MapView/MapView.js`:
- Around line 9-10: MapView currently statically imports react-leaflet
(useMapEvents and the ReactLeaflet namespace) and related components like
UserMarker, causing SSR to evaluate Leaflet code and crash; change this by
removing those top-level imports and instead load the entire map subtree on the
client only — either dynamically import the MapView map subtree (and components
such as UserMarker) with SSR disabled (e.g., next/dynamic({ ssr: false })) or
render the map subtree only after a client mount check (useEffect/setState) so
useMapEvents and ReactLeaflet are only imported/executed in the browser; ensure
all references to useMapEvents, ReactLeaflet, and UserMarker are moved into the
client-only module or the dynamically imported component.
---
Nitpick comments:
In `@package.json`:
- Line 72: The lint:fix npm script is missing the .mjs extension so autofixes
can miss files; update the "lint:fix" script in package.json (the lint:fix
entry) to include --ext .js,.jsx,.mjs for eslint and ensure prettier invocation
also targets .mjs (make the extensions consistent with the "lint" script) so
.mjs files are linted and auto-fixed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 459a2113-8509-4bad-96d5-c8bdc3c4977b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
.babelrc.eslintrc.jsonDockerfile__mocks__/withStyles.jsclient/client.jscompose.ymlconfig/index.jsconfig/package.jsondocs/ViteSSRMigrationPlan.mdindex.htmlpackage.jsonscripts/package.jsonscripts/update-runtime-env.jsserver.mjsserver/ieMiddleware.jsserver/server-entry.jsserver/server.jsserver/sitemapMiddlewares.jssrc/components/ListItems/SimpleListItem/SimpleListItem.jssrc/components/ListItems/SuggestionItem/SuggestionItem.jssrc/components/NewsInfo/components/NewsItem/NewsItem.jssrc/components/SearchBar/SearchBarComponent.jssrc/components/TabLists/TabLists.jssrc/components/TopBar/DrawerMenu/DrawerMenu.jssrc/createEmotionCache.jssrc/entry-client.jsxsrc/entry-server.jsxsrc/layouts/EmbedLayout.jssrc/store.jssrc/views/AreaView/components/ServiceFilterContainer/ServiceFilterContainer.jssrc/views/FeedbackView/FeedbackView.jssrc/views/MapView/MapView.jssrc/views/MapView/components/AddressMarker/AddressMarker.jssrc/views/MapView/components/Districts/ParkingAreas.jssrc/views/MapView/components/HideSidebarButton/HideSidebarButton.jssrc/views/MapView/components/MarkerCluster/MarkerCluster.jssrc/views/MapView/components/TransitStops/TransitStops.jssrc/views/MapView/components/UserMarker/UserMarker.jssrc/views/MapView/utils/createMap.jssrc/views/MapView/utils/drawIcon.jssrc/views/MapView/utils/mapActions.jssrc/views/MapView/utils/swapCoordinates.jssrc/views/MapView/utils/transitFetch.jsvite.config.jsvitest.config.js
💤 Files with no reviewable changes (5)
- server/server.js
- .babelrc
- client/client.js
- mocks/withStyles.js
- server/ieMiddleware.js
607e81a to
cf1dd83
Compare
|
PALVELUKARTTA-UI branch is deployed to platta: https://palvelukartta-ui-pr1360.dev.hel.ninja 🚀🚀🚀 |
2 similar comments
|
PALVELUKARTTA-UI branch is deployed to platta: https://palvelukartta-ui-pr1360.dev.hel.ninja 🚀🚀🚀 |
|
PALVELUKARTTA-UI branch is deployed to platta: https://palvelukartta-ui-pr1360.dev.hel.ninja 🚀🚀🚀 |
|
a1d8ebe to
e4fd410
Compare
|
|
PALVELUKARTTA-UI branch is deployed to platta: https://palvelukartta-ui-pr1360.dev.hel.ninja 🚀🚀🚀 |
|
PALVELUKARTTA-UI branch is deployed to platta: https://palvelukartta-ui-pr1360.dev.hel.ninja 🚀🚀🚀 |



The current SSR implementation is really heavy. Migrated the application to use Vite Server-Side Rendering instead.
Azure: https://dev.azure.com/City-of-Helsinki/palvelukartta/_git/palvelukartta-pipelines/pullrequest/16188
Summary by CodeRabbit
Release Notes
New Features
Improvements
Removals