Yuki Takei hace 3 semanas
padre
commit
a427abfe7b

+ 23 - 66
.kiro/specs/optimise-deps-for-prod/design.md

@@ -169,49 +169,29 @@ Each phase gate requires: production server starts without errors + `GET /` retu
 - Any package appearing in `.next/node_modules/` after a production build must be in `dependencies`, not `devDependencies`.
 - Changes propagate to all consumers of the monorepo lock file; `pnpm install --frozen-lockfile` must remain valid.
 
-**Phase 1 changes — move all 23 to `dependencies`**:
-
-| Package | Current | Target | Rationale |
-|---------|---------|--------|-----------|
-| `@codemirror/state` | devDep | dep | Used in editor components (SSR'd) |
-| `@emoji-mart/data` | devDep | dep | Static import in remark plugin (server-side) |
-| `@handsontable/react` | devDep | dep | Used in HandsontableModal (SSR'd unless wrapped) |
-| `@headless-tree/core` | devDep | dep | Used in PageTree hooks (SSR'd) |
-| `@headless-tree/react` | devDep | dep | Used in ItemsTree (SSR'd) |
-| `@tanstack/react-virtual` | devDep | dep | Used in ItemsTree (layout-critical, SSR'd) |
-| `bootstrap` | devDep | dep | Dynamic JS import in `_app.page.tsx` (Phase 4 to verify) |
-| `diff2html` | devDep | dep | Used in RevisionDiff (SSR'd) |
-| `downshift` | devDep | dep | Used in SearchModal (SSR'd) |
-| `fastest-levenshtein` | devDep | dep | Used in openai client service (SSR'd) |
-| `fslightbox-react` | devDep | dep | Used in LightBox (SSR'd) |
-| `i18next-http-backend` | devDep | dep | Present in `.next/node_modules/`; source unknown (Phase 4 to verify) |
-| `i18next-localstorage-backend` | devDep | dep | Present in `.next/node_modules/`; source unknown (Phase 4 to verify) |
-| `pretty-bytes` | devDep | dep | Used in RichAttachment (SSR'd) |
-| `react-copy-to-clipboard` | devDep | dep | Used in multiple inline components (SSR'd) |
-| `react-dnd` | devDep | dep | Used in PageTree drag-drop (SSR'd) |
-| `react-dnd-html5-backend` | devDep | dep | Used in PageTree drag-drop (SSR'd) |
-| `react-dropzone` | devDep | dep | Present in `.next/node_modules/`; source unknown (Phase 4 to verify) |
-| `react-hook-form` | devDep | dep | Used in forms across app (SSR'd) |
-| `react-input-autosize` | devDep | dep | Used in form inputs (SSR'd) |
-| `react-toastify` | devDep | dep | Static import in `toastr.ts` (SSR'd) |
-| `simplebar-react` | devDep | dep | Used in Sidebar, AiAssistant (layout-critical, SSR'd) |
-| `socket.io-client` | devDep | dep | Static import in admin socket atom (Phase 4 refactor) |
-
-**Phase 2–4 revert candidates** (move back to `devDependencies` if conditions met):
-
-| Package | Condition for revert | Phase |
-|---------|----------------------|-------|
-| `@emoji-mart/data` | Server-side import removed or replaced with static file | 2 |
-| `fslightbox-react` | Wrapped with `dynamic({ ssr: false })`; no longer in `.next/node_modules/` | 3 |
-| `diff2html` | Wrapped with `dynamic({ ssr: false })`; no longer in `.next/node_modules/` | 3 |
-| `react-dnd` | DnD-specific components wrapped with `dynamic({ ssr: false })` | 3 |
-| `react-dnd-html5-backend` | Same as `react-dnd` | 3 |
-| `@handsontable/react` | Confirmed `dynamic({ ssr: false })` in HandsontableModal | 3 |
-| `socket.io-client` | Admin socket refactored to dynamic import | 4 |
-| `bootstrap` | Confirmed `import()` is browser-only (inside `useEffect`) | 4 |
-| `i18next-http-backend` | Confirmed absent from `.next/node_modules/` post-Phase-1 | 4 |
-| `i18next-localstorage-backend` | Confirmed absent from `.next/node_modules/` post-Phase-1 | 4 |
-| `react-dropzone` | Confirmed absent from `.next/node_modules/` post-Phase-1 | 4 |
+**Final classification** (all packages in `dependencies`; 3 successfully reverted to `devDependencies`):
+
+| Package | Final classification | Rationale |
+|---------|---------------------|-----------|
+| `@codemirror/state` + 18 others (`codemirror`, `codemirror-emacs/vim/vscode-keymap`, `@lezer/highlight`, `@marp-team/*`, `@emoji-mart/react`, `reveal.js`, `pako`, `cm6-theme-basic-light`, `y-codemirror.next`) | `dependencies` | Found in `.next/node_modules/` via transitive imports; not in initial 23 — discovered during implementation |
+| `@handsontable/react` | `dependencies` | Still externalised despite `useEffect` dynamic import; `ssr: false` does not prevent Turbopack externalisation |
+| `@headless-tree/core`, `@headless-tree/react` | `dependencies` | Used in PageTree hooks (SSR'd) |
+| `@tanstack/react-virtual` | `dependencies` | Used in ItemsTree (layout-critical, SSR'd) |
+| `bootstrap` | `dependencies` | Still externalised despite `useEffect`-guarded `import()`; transitive imports cause externalisation |
+| `diff2html` | `dependencies` | Still externalised despite `ssr: false` on RevisionDiff; static import analysis reaches it |
+| `downshift` | `dependencies` | Used in SearchModal (SSR'd) |
+| `fastest-levenshtein` | `dependencies` | Used in openai client service (SSR'd) |
+| `i18next-http-backend`, `i18next-localstorage-backend`, `react-dropzone` | `dependencies` | Still in `.next/node_modules/` via transitive imports despite no direct `src/` imports |
+| `pretty-bytes` | `dependencies` | Used in RichAttachment (SSR'd) |
+| `react-copy-to-clipboard` | `dependencies` | Used in multiple inline components (SSR'd) |
+| `react-dnd`, `react-dnd-html5-backend` | `dependencies` | Still externalised despite DnD provider wrapped with `ssr: false` |
+| `react-hook-form` | `dependencies` | Used in forms across app (SSR'd) |
+| `react-input-autosize` | `dependencies` | Used in form inputs (SSR'd) |
+| `react-toastify` | `dependencies` | Static `{ toast }` import in `toastr.ts`; async refactor would change API surface |
+| `simplebar-react` | `dependencies` | Used in Sidebar, AiAssistant (layout-critical, SSR'd) |
+| **`@emoji-mart/data`** | **`devDependencies`** | Replaced with bundled `emoji-native-lookup.json`; no runtime package needed |
+| **`fslightbox-react`** | **`devDependencies`** | `import()` inside `useEffect`; broken symlink in `.next/node_modules/` is harmless (SSR never accesses it) |
+| **`socket.io-client`** | **`devDependencies`** | `await import()` inside `useEffect` in `admin/states/socket-io.ts`; no static SSR import path |
 
 **Contracts**: State [x]
 
@@ -465,26 +445,3 @@ done
 
 ---
 
-## Migration Strategy
-
-The five phases are executed sequentially. Each phase is independently deployable and verifiable.
-
-```mermaid
-graph LR
-    P1[Phase 1\nBaseline fix\nall 23 to deps]
-    P2[Phase 2\n@emoji-mart/data\nserver import]
-    P3[Phase 3\nssr:false\nGroup 1 candidates]
-    P4[Phase 4\nAmbiguous\npackages]
-    P5[Phase 5\nValidation\ndocumentation]
-
-    P1 --> P2
-    P2 --> P3
-    P3 --> P4
-    P4 --> P5
-```
-
-**Rollback**: Each phase modifies only `package.json` and/or one source file. Rolling back is a targeted revert of those changes; the production build pipeline (`assemble-prod.sh`, Dockerfile) is unchanged throughout.
-
-**Phase 1 rollback trigger**: Production server fails to start or CI `launch-prod` fails → revert `package.json` changes.
-
-**Phase 3/4 rollback trigger**: Hydration error or functional regression detected → revert the specific `dynamic()` wrapping or import refactor; package remains in `dependencies`.

+ 5 - 34
.kiro/specs/optimise-deps-for-prod/requirements.md

@@ -14,13 +14,7 @@ The current `pnpm deploy --prod` step excludes `devDependencies`, causing missin
 
 **Objective:** As a release engineer, I want `pnpm deploy --prod` to produce a complete, self-contained production artifact, so that the production server starts without missing-module errors.
 
-#### Acceptance Criteria
-
-1. The build system shall move all 23 packages currently appearing in `.next/node_modules/` but classified as `devDependencies` into `dependencies` in `apps/app/package.json`.
-2. When `pnpm deploy out --prod --legacy --filter @growi/app` is executed, the output `node_modules` shall contain every package referenced by `.next/node_modules/` symlinks.
-3. When the production server is started with `pnpm run server`, the build system shall not throw `ERR_MODULE_NOT_FOUND` or `Failed to load external module` errors on any page request.
-4. When a browser accesses the `/login` page, the GROWI server shall respond with HTTP 200 and render the login page without SSR errors in the server log.
-5. The GROWI server shall pass existing CI smoke tests (`launch-prod` job) after this change.
+**Summary**: All packages appearing in `.next/node_modules/` after a Turbopack production build must be in `dependencies`. The production server must start without `ERR_MODULE_NOT_FOUND` errors and pass the `launch-prod` CI job against MongoDB 6.0 and 8.0.
 
 ---
 
@@ -28,12 +22,7 @@ The current `pnpm deploy --prod` step excludes `devDependencies`, causing missin
 
 **Objective:** As a developer, I want to remove direct server-side imports of packages that do not need to run on the server, so that those packages can revert to `devDependencies` and be excluded from the production artifact.
 
-#### Acceptance Criteria
-
-1. When `@emoji-mart/data` is investigated, the build system shall determine whether its import in `services/renderer/remark-plugins/emoji.ts` can be replaced with a server-safe alternative (e.g., a bundled subset of emoji data, or a lazy `require()` that avoids Turbopack externalisation).
-2. If a viable server-side import removal is identified for a Group 2 package, the GROWI server shall continue to render emoji correctly in Markdown output after the refactor.
-3. If a Group 2 package's server-side import is successfully removed and it no longer appears in `.next/node_modules/` after a production build, the build system shall move that package back to `devDependencies`.
-4. If removal of a server-side import is not viable without breaking functionality, the package shall remain in `dependencies` and be documented as a justified production dependency.
+**Summary**: Investigate whether `@emoji-mart/data`'s server-side import in `emoji.ts` can be replaced with a server-safe alternative. If successful, emoji rendering must produce identical output and the package must revert to `devDependencies`. If removal is not viable, document as a justified production dependency.
 
 ---
 
@@ -41,13 +30,7 @@ The current `pnpm deploy --prod` step excludes `devDependencies`, causing missin
 
 **Objective:** As a developer, I want to wrap client-only UI components with `dynamic(() => import(...), { ssr: false })` where appropriate, so that their dependencies are excluded from Turbopack's SSR externalisation and can revert to `devDependencies`.
 
-#### Acceptance Criteria
-
-1. When a Group 1 package is evaluated, the build system shall determine whether its consuming component(s) can be safely rendered client-side only (no meaningful content lost from initial HTML, no SEO impact, no hydration mismatch risk).
-2. Where a component is safe for `ssr: false`, the GROWI app shall wrap the component import with `dynamic(() => import('...'), { ssr: false })` and the package shall no longer appear in `.next/node_modules/` after a production build.
-3. When `ssr: false` is applied to a component, the GROWI app shall not exhibit hydration errors or visible layout shift in the browser.
-4. If a Group 1 package's component is successfully converted to `ssr: false` and no longer appears in `.next/node_modules/`, the build system shall move that package back to `devDependencies`.
-5. If a component cannot be wrapped with `ssr: false` without breaking functionality or user experience, the package shall remain in `dependencies` and be documented as a justified production dependency.
+**Summary**: Evaluate Group 1 components for `ssr: false` safety (no SEO impact, no hydration mismatch, no visible layout shift). Successfully converted components must no longer appear in `.next/node_modules/`; otherwise, the package stays in `dependencies` with documentation.
 
 ---
 
@@ -55,13 +38,7 @@ The current `pnpm deploy --prod` step excludes `devDependencies`, causing missin
 
 **Objective:** As a developer, I want to determine the correct final classification for packages with unclear or mixed usage patterns, so that every entry in `dependencies` and `devDependencies` has a documented and verified rationale.
 
-#### Acceptance Criteria
-
-1. When `react-toastify` is investigated, the build system shall determine whether the direct import in `client/util/toastr.ts` causes Turbopack to externalise it independently of the `ssr: false`-guarded `ToastContainer`, and classify accordingly.
-2. When `socket.io-client` is investigated, the build system shall determine whether the direct import in `features/admin/states/socket-io.ts` requires the package at SSR runtime, and either refactor it to a dynamic import or document it as a justified production dependency.
-3. When `bootstrap` is investigated, the build system shall determine whether a JavaScript import (beyond SCSS) causes it to appear in `.next/node_modules/`, and classify accordingly.
-4. When `i18next-http-backend`, `i18next-localstorage-backend`, and `react-dropzone` are investigated and found to have no direct imports in `src/`, the build system shall determine whether they appear in `.next/node_modules/` via transitive dependencies and remove them from `devDependencies` entirely if they are unused.
-5. The GROWI server shall pass existing CI smoke tests after all reclassifications in this requirement are applied.
+**Summary**: Resolve `react-toastify`, `socket.io-client`, `bootstrap`, and phantom packages (`i18next-http-backend`, `i18next-localstorage-backend`, `react-dropzone`) through code analysis and build verification. Refactor to dynamic imports where feasible; otherwise document as justified production dependencies. CI must pass after all reclassifications.
 
 ---
 
@@ -69,10 +46,4 @@ The current `pnpm deploy --prod` step excludes `devDependencies`, causing missin
 
 **Objective:** As a release engineer, I want a verified and documented final state of `dependencies` vs `devDependencies`, so that future package additions follow the correct classification rules.
 
-#### Acceptance Criteria
-
-1. The GROWI build system shall produce a production artifact where every package in `.next/node_modules/` is resolvable from `apps/app/node_modules/` (i.e., no broken symlinks in the release image).
-2. The GROWI server shall start and serve the login page without errors after a full `pnpm deploy --prod` cycle.
-3. The `apps/app/package.json` shall contain no packages in `devDependencies` that appear in `.next/node_modules/` after a production build.
-4. The build system shall pass the full `launch-prod` CI job including MongoDB connectivity checks.
-5. The GROWI codebase shall include a comment or documentation entry explaining the Turbopack externalisation rule: any package imported in SSR-rendered code (including Pages Router components and server-side utilities) must be in `dependencies`, not `devDependencies`.
+**Summary**: Every symlink in `.next/node_modules/` must resolve correctly in the release artifact (no broken symlinks). No `devDependencies` package may appear in `.next/node_modules/` after a production build. The codebase must include documentation of the Turbopack externalisation rule to prevent future misclassification.

+ 57 - 0
.kiro/specs/optimise-deps-for-prod/research.md

@@ -139,3 +139,60 @@
 - pnpm deploy documentation — `pnpm deploy` flags and node-linker behaviour
 - Next.js Pages Router SSR — all pages render server-side by default; `dynamic({ ssr: false })` is the only opt-out
 - Turbopack externalisation — packages in `.next/node_modules/` are loaded at runtime, not bundled
+
+---
+
+## Session 2: Production Implementation Discoveries
+
+### Finding: `ssr: false` does NOT prevent Turbopack externalisation
+
+**Pre-implementation assumption**: Wrapping a component with `dynamic({ ssr: false })` would remove its package dependencies from `.next/node_modules/`.
+
+**Reality**: Turbopack performs static import analysis on the dynamically-loaded file and still externalises packages found there. `ssr: false` only skips HTML rendering — it does not affect which packages are added to `.next/node_modules/`. This invalidated the entire Phase 3 plan (wrapping `diff2html`, `react-dnd`, `@handsontable/react` with `ssr: false`).
+
+**Only two techniques actually remove a package from `.next/node_modules/`**:
+1. Replace the static import with `import()` inside `useEffect` and ensure no other static import path exists in the SSR code graph (e.g., `socket.io-client` in `admin/states/socket-io.ts`).
+2. Replace the runtime package with a bundled static alternative (e.g., `@emoji-mart/data` → `emoji-native-lookup.json`).
+
+**Exception**: `fslightbox-react` remains in `.next/node_modules/` as a broken symlink but is harmless — `useEffect` never runs during SSR, so the broken symlink is never accessed.
+
+---
+
+### Finding: Initial survey of 23 packages was incomplete
+
+The design identified 23 packages to move from `devDependencies` to `dependencies`. During implementation, 19 additional packages were found in `.next/node_modules/`:
+
+- `@codemirror/*` (multiple packages), `codemirror`, `codemirror-emacs`, `codemirror-vim`, `codemirror-vscode-keymap`
+- `@lezer/highlight`
+- `@marp-team/marp-core`, `@marp-team/marpit`
+- `@emoji-mart/react`
+- `reveal.js`, `pako`, `cm6-theme-basic-light`, `y-codemirror.next`
+
+All 42 packages (23 + 19) were moved to `dependencies`. Lesson: always run the Level 1 check (`ls apps/app/.next/node_modules/`) after a production build to get the authoritative list.
+
+---
+
+### Finding: `assemble-prod.sh` had two bugs
+
+1. **`set -e` + `[ ... ] && ...` pattern**: Under `set -e`, a `[ condition ] && command` expression exits the script with failure when the condition is false (exit code 1). Fixed by wrapping in `if/then`.
+2. **`.next/node_modules/` symlink rewrite was missing**: The script rewrote `apps/app/node_modules/` symlinks but did not rewrite `.next/node_modules/` symlinks. These still pointed to `../../../../node_modules/.pnpm/` (workspace root), which does not exist in production. Both rewrites are now present.
+
+---
+
+### Finding: Packages successfully reverted to `devDependencies`
+
+Only 3 of the 23 originally moved packages were successfully reverted:
+
+| Package | Technique |
+|---------|-----------|
+| `@emoji-mart/data` | Replaced with bundled `emoji-native-lookup.json` extracted at build time |
+| `fslightbox-react` | Replaced static import with `import()` inside `useEffect` in `LightBox.tsx` |
+| `socket.io-client` | Replaced static import with `await import()` inside `useEffect` in `admin/states/socket-io.ts` |
+
+All other packages remain in `dependencies` because either `ssr: false` wrapping failed to remove them from `.next/node_modules/` or they are genuinely needed at SSR runtime.
+
+---
+
+### Finding: CI symlink integrity check added
+
+`check-next-symlinks.sh` was added to the `build-prod` CI job (runs after `assemble-prod.sh`) to detect broken symlinks in `.next/node_modules/` automatically. This prevents future classification regressions regardless of which code paths are exercised at runtime by `server:ci`. The `fslightbox-react` exception is hardcoded in the script.

+ 3 - 2
.kiro/specs/optimise-deps-for-prod/spec.json

@@ -1,9 +1,10 @@
 {
   "feature_name": "optimise-deps-for-prod",
   "created_at": "2026-03-12T05:00:00Z",
-  "updated_at": "2026-03-12T05:20:00Z",
+  "updated_at": "2026-03-16T00:00:00Z",
   "language": "en",
-  "phase": "tasks-generated",
+  "phase": "implementation-complete",
+  "cleanup_completed": true,
   "approvals": {
     "requirements": {
       "generated": true,