Extract framework-agnostic lesson logic into @spelling-creator/core - #16
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (29)
📝 WalkthroughWalkthroughThe change adds ChangesShared core package migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EditorPage
participant jsonExport
participant jsonImport
participant BrowserAPIs
participant HubPage
participant lessonSearch
participant FuseJS
EditorPage->>jsonExport: exportJson(doc)
jsonExport->>BrowserAPIs: create JSON blob and trigger download
EditorPage->>jsonImport: importJsonFile(file)
jsonImport->>BrowserAPIs: read and parse file
jsonImport-->>EditorPage: normalized lesson document
HubPage->>lessonSearch: buildLessonIndex(lessons)
lessonSearch->>FuseJS: index title and author fields
HubPage->>lessonSearch: searchLessons(index, query)
lessonSearch-->>HubPage: ranked lesson results
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
playforge-coding
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.oxfmtrc.json (1)
13-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
.oxfmtrc.jsoncfor comments.Rename the file to
.oxfmtrc.jsonc, or remove the comments to keep.oxfmtrc.jsonas standard JSON.🤖 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 @.oxfmtrc.json around lines 13 - 14, Update the formatter configuration by renaming .oxfmtrc.json to .oxfmtrc.jsonc so the existing comment and configuration remain valid; alternatively, remove the comment and retain the .json filename as strict JSON.Sources: Coding guidelines, Linters/SAST tools
🤖 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 `@apps/docs/docs/monorepo/overview.md`:
- Around line 13-18: Add an apps/docs row to the monorepo package table,
identifying it as `@spelling-creator/docs` and describing its documentation site
consistently with the existing README and deployment configuration.
In `@apps/docs/docs/web-app/project-structure.md`:
- Line 101: Add the text language tag to the fenced code block at the affected
documentation section, changing the opening fence to specify text while leaving
the directory listing content unchanged.
In `@packages/core/src/jsonImport.js`:
- Around line 62-70: Update normalizeLessonFile to validate the envelope version
alongside parsed.format, rejecting any present version that differs from
LESSON_FILE_VERSION before selecting docLike or normalizing. Preserve acceptance
of bare legacy documents with no format or version, and keep the existing
wrong-format JsonImportError behavior.
---
Nitpick comments:
In @.oxfmtrc.json:
- Around line 13-14: Update the formatter configuration by renaming
.oxfmtrc.json to .oxfmtrc.jsonc so the existing comment and configuration remain
valid; alternatively, remove the comment and retain the .json filename as strict
JSON.
🪄 Autofix
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 Plus
Run ID: 543fa5e8-d14c-447a-a977-ba97d4f7265f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.github/workflows/deploy.yml.oxfmtrc.json.oxlintrc.jsonREADME.mdapps/docs/docs/monorepo/getting-started.mdapps/docs/docs/monorepo/overview.mdapps/docs/docs/web-app/project-structure.mdapps/web/package.jsonapps/web/src/components/AiQuestionDialog.jsxapps/web/src/components/ContentBlock.jsxapps/web/src/components/LessonView.jsxapps/web/src/components/MergeDialog.jsxapps/web/src/components/SectionCard.jsxapps/web/src/lib/collab.jsapps/web/src/lib/docxExport.jsapps/web/src/lib/docxImport.jsapps/web/src/lib/git/sync.jsapps/web/src/lib/htmlPreview.jsapps/web/src/pages/EditorPage.jsxapps/web/src/pages/HubPage.jsxpackages/core/package.jsonpackages/core/src/ageRanges.jspackages/core/src/id.jspackages/core/src/image.jspackages/core/src/jsonExport.jspackages/core/src/jsonImport.jspackages/core/src/lessonSearch.jspackages/core/src/questions.jspackages/core/src/spelling.jspnpm-workspace.yaml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.oxfmtrc.json (1)
13-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
.oxfmtrc.jsoncfor comments.Rename the file to
.oxfmtrc.jsonc, or remove the comments to keep.oxfmtrc.jsonas standard JSON.🤖 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 @.oxfmtrc.json around lines 13 - 14, Update the formatter configuration by renaming .oxfmtrc.json to .oxfmtrc.jsonc so the existing comment and configuration remain valid; alternatively, remove the comment and retain the .json filename as strict JSON.Sources: Coding guidelines, Linters/SAST tools
🤖 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 `@apps/docs/docs/monorepo/overview.md`:
- Around line 13-18: Add an apps/docs row to the monorepo package table,
identifying it as `@spelling-creator/docs` and describing its documentation site
consistently with the existing README and deployment configuration.
In `@apps/docs/docs/web-app/project-structure.md`:
- Line 101: Add the text language tag to the fenced code block at the affected
documentation section, changing the opening fence to specify text while leaving
the directory listing content unchanged.
In `@packages/core/src/jsonImport.js`:
- Around line 62-70: Update normalizeLessonFile to validate the envelope version
alongside parsed.format, rejecting any present version that differs from
LESSON_FILE_VERSION before selecting docLike or normalizing. Preserve acceptance
of bare legacy documents with no format or version, and keep the existing
wrong-format JsonImportError behavior.
---
Nitpick comments:
In @.oxfmtrc.json:
- Around line 13-14: Update the formatter configuration by renaming
.oxfmtrc.json to .oxfmtrc.jsonc so the existing comment and configuration remain
valid; alternatively, remove the comment and retain the .json filename as strict
JSON.
🪄 Autofix
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 Plus
Run ID: 543fa5e8-d14c-447a-a977-ba97d4f7265f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.github/workflows/deploy.yml.oxfmtrc.json.oxlintrc.jsonREADME.mdapps/docs/docs/monorepo/getting-started.mdapps/docs/docs/monorepo/overview.mdapps/docs/docs/web-app/project-structure.mdapps/web/package.jsonapps/web/src/components/AiQuestionDialog.jsxapps/web/src/components/ContentBlock.jsxapps/web/src/components/LessonView.jsxapps/web/src/components/MergeDialog.jsxapps/web/src/components/SectionCard.jsxapps/web/src/lib/collab.jsapps/web/src/lib/docxExport.jsapps/web/src/lib/docxImport.jsapps/web/src/lib/git/sync.jsapps/web/src/lib/htmlPreview.jsapps/web/src/pages/EditorPage.jsxapps/web/src/pages/HubPage.jsxpackages/core/package.jsonpackages/core/src/ageRanges.jspackages/core/src/id.jspackages/core/src/image.jspackages/core/src/jsonExport.jspackages/core/src/jsonImport.jspackages/core/src/lessonSearch.jspackages/core/src/questions.jspackages/core/src/spelling.jspnpm-workspace.yaml
🛑 Comments failed to post (1)
packages/core/src/jsonImport.js (1)
62-70: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject unsupported lesson-file versions.
normalizeLessonFilevalidatesformatbut ignoresversion. A future incompatible envelope can pass normalization, lose unsupported fields, and then overwrite the source when the user saves it.Reject envelope versions other than
LESSON_FILE_VERSION. Continue to accept bare legacy documents that have noformat.Proposed fix
-import { LESSON_FILE_FORMAT } from "./jsonExport.js"; +import { + LESSON_FILE_FORMAT, + LESSON_FILE_VERSION, +} from "./jsonExport.js"; ... if (parsed.format && parsed.format !== LESSON_FILE_FORMAT) { throw new JsonImportError( `This JSON file is “${parsed.format}”, not a Spelling Creator lesson.`, ); } + if ( + parsed.format === LESSON_FILE_FORMAT && + parsed.version !== LESSON_FILE_VERSION + ) { + throw new JsonImportError( + `This lesson file uses unsupported version “${parsed.version}”.`, + ); + }📝 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.// A wrong but explicit format marker is a clear, actionable error. if (parsed.format && parsed.format !== LESSON_FILE_FORMAT) { throw new JsonImportError( `This JSON file is “${parsed.format}”, not a Spelling Creator lesson.`, ); } if ( parsed.format === LESSON_FILE_FORMAT && parsed.version !== LESSON_FILE_VERSION ) { throw new JsonImportError( `This lesson file uses unsupported version “${parsed.version}”.`, ); } const docLike = parsed.doc && typeof parsed.doc === "object" ? parsed.doc : parsed; return normalizeDoc(docLike);🤖 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 `@packages/core/src/jsonImport.js` around lines 62 - 70, Update normalizeLessonFile to validate the envelope version alongside parsed.format, rejecting any present version that differs from LESSON_FILE_VERSION before selecting docLike or normalizing. Preserve acceptance of bare legacy documents with no format or version, and keep the existing wrong-format JsonImportError behavior.
This was referenced Aug 5, 2026
The lesson model is currently spread across apps/web/src/lib, which means the Worker and the MCP server cannot reuse any of it. This adds packages/core and moves the first tier out: the modules with no React dependency and no imports back into the app. All eight moves are pure renames — no content changed, so the diff is the import rewrites and the package wiring. The package has no build step and no barrel entry, matching the convention @spelling-creator/mcp already uses: every module is its own subpath export. That keeps browser-only modules out of the Worker's import graph, so importing /questions can never drag in something that touches document. Two of the moved modules (image, jsonExport) do still reach for the DOM, and only the web app imports them today. Rather than grant core the browser env wholesale, it is linted against the narrower worker env and those two files are opted back into browser by name — so anything added later that reaches for a browser global fails lint instead of breaking inside the Worker. They move behind a /browser subpath when the rest of that tier is extracted. fuse.js moves with lessonSearch to core's dependencies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
playforge-coding
force-pushed
the
core-extract-leaves
branch
from
August 5, 2026 23:30
f2cd1b5 to
4b7e0e1
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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.
First layer of splitting the lesson model out of
apps/web/src/libso the Worker and the MCP server can reuse it.Adds
packages/coreand moves the tier that has no React dependency and no imports back into the app. All eight moves are pure renames — no content changed — so the reviewable diff is the import rewrites and the package wiring.Shape
No build step, no barrel entry: every module is its own subpath export, matching the convention
@spelling-creator/mcpalready uses. That is what keeps browser-only modules out of the Worker's import graph — importing/questionscan never drag in something that touchesdocument.The lint rule is the interesting part
Two of the moved modules (
image,jsonExport) still reach for the DOM. Rather than grant core the browser env wholesale, it is linted against the narrowerworkerenv with those two files opted back intobrowserby name. So anything added to core later that reaches for a browser global fails lint instead of breaking inside the Worker. They move behind a/browsersubpath when the rest of that tier is extracted.Deliberately left behind
imageRef.js(pulls inimageStore.js/ IndexedDB) andhtmlPreview.js(pulls indocx/mammoth) — browser tier, not leaf tier.spellingWords.js— readsimport.meta.env.VITE_API_URLat module scope, so it breaks outside an rsbuild build. Needs a refactor, not a move.i18n.js/languages.js— coupled toreact-i18next.fuse.jsmoves withlessonSearchto core's dependencies.Also here
A separate commit adds
.agents/skills/**to oxfmt's ignore patterns. That break is pre-existing onmaster(vendored skill content is formatted upstream) and would otherwise fail CI for every PR in this stack.Verification
pnpm run lint,pnpm run build,pnpm run build:docs, and all three suites — web 15, api 27, mcp 14 — pass.Stack created with GitHub Stacks CLI • Give Feedback 💬
Summary by CodeRabbit