Markdown-first CLI, local engines/mirror, HTML export, and Electron + React scaffold - #4
Merged
YellowKidokc merged 2 commits intoJun 13, 2026
Conversation
|
Warning Review limit reached
More reviews will be available in 40 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR introduces a complete Electron + React desktop application implementing the Forge markdown-first document system with typed block identification, enrichment layers (claims/commentary), pluggable engines for export/statistics generation, and a grid-addressable renderer. The codebase spans markdown parsing with collision-aware ID generation, sidecar JSON layer/claims loading, comprehensive validation, a CLI dispatcher routing eight subcommands, Electron window management with IPC, and a full React UI with layer-aware rendering, word-level grid selection, and semantic tagging. ChangesForge Document Editing & Rendering Platform
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57e7cf5257
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
exports/article.html-148-153 (1)
148-153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRedact the checked-in sample's local path and timestamp.
This export bakes in a workspace-specific absolute path and an exact export timestamp, so the fixture leaks local filesystem details and will churn on every regeneration. Use a relative/redacted source label and a stable placeholder if this file is meant to stay committed.
🤖 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 `@exports/article.html` around lines 148 - 153, Replace the workspace-specific absolute path and exact export timestamp in exports/article.html: update the <dd> value for the "Source" entry (currently "/workspace/Forge-electron/samples/article.md") to a redacted or relative form like "samples/article.md" or "<redacted>/samples/article.md", and change the <dd> value for "Export timestamp" (currently "2026-06-11T12:23:29.520Z") to a stable placeholder such as "EXPORT_TIMESTAMP" or a fixed ISO stub ("1970-01-01T00:00:00Z") so the fixture no longer leaks local filesystem details or churns on regeneration; ensure only the text nodes for those <dd> entries are modified and leave "Document ID", "Block count", "Layer file", and "Claims file" entries unchanged.README.md-41-41 (1)
41-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winImprove phrasing at Line 41 for readability.
The sentence is a bit awkward; tightening wording will read more professionally in top-level docs.
Suggested edit
-The desktop shell is scaffolded with Electron + electron-vite + React + TypeScript. It opens a frameless dark FORGE window with a custom topbar, sidebar article switcher, content surface, and footer. +The desktop shell is scaffolded with Electron + electron-vite + React + TypeScript. It opens a frameless FORGE window with a dark theme, custom top bar, article-switching sidebar, content surface, and footer.🤖 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 `@README.md` at line 41, Update the sentence that starts "The desktop shell is scaffolded with Electron + electron-vite + React + TypeScript." to a tighter, more professional phrasing: replace it with something like "The desktop shell is built with Electron, electron-vite, React, and TypeScript; it launches a frameless, dark FORGE window with a custom topbar, sidebar article switcher, content area, and footer." Locate and edit the sentence (the line beginning "The desktop shell is scaffolded...") in README.md.Source: Linters/SAST tools
src/cli/forge.ts-37-41 (1)
37-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire an actual value after
--vault/--mirror.Current parsing accepts the next flag token as a path when the value is omitted, which can send operations to unintended locations.
Suggested fix
function optionValue(flags: string[], name: string): string | undefined { const index = flags.indexOf(name); if (index === -1) return undefined; - return flags[index + 1]; + const value = flags[index + 1]; + if (!value || value.startsWith('--')) { + throw new Error(`${name} requires a value`); + } + return value; }🤖 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/cli/forge.ts` around lines 37 - 41, The optionValue function currently treats the next token as a value even if it's another flag; update optionValue(flags: string[], name: string) to verify that index+1 exists and that flags[index + 1] is a real value (not empty and does not start with '-') and return undefined otherwise; modify the lookup in optionValue (and any callers that rely on it, e.g., parsing for --vault / --mirror) so omitted values are treated as missing rather than using the next flag token.src/export/htmlExporter.ts-251-258 (1)
251-258:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle clipboard failures explicitly in copy-link behavior.
The click handler does not catch clipboard errors and can misreport success when copying is unavailable/denied.
Suggested fix
document.querySelectorAll('.copy-link').forEach((button) => { button.addEventListener('click', async () => { const target = button.dataset.copyTarget; const url = new URL(window.location.href); url.hash = target; - await navigator.clipboard?.writeText(url.toString()); - button.textContent = 'Copied'; - setTimeout(() => { button.textContent = 'Copy link'; }, 1200); + try { + if (!navigator.clipboard?.writeText) throw new Error('Clipboard unavailable'); + await navigator.clipboard.writeText(url.toString()); + button.textContent = 'Copied'; + } catch { + button.textContent = 'Copy failed'; + } finally { + setTimeout(() => { button.textContent = 'Copy link'; }, 1200); + } }); });🤖 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/export/htmlExporter.ts` around lines 251 - 258, Wrap the clipboard write in a try/catch inside the button.click handler that uses the existing dataset.copyTarget and URL-building logic; await navigator.clipboard.writeText(url.toString()) inside the try and only set button.textContent = 'Copied' on success, while in the catch set a failure state (e.g., 'Copy failed' or a permission message), log the error (console.error or process logger), and restore the original 'Copy link' text after the timeout so the UI doesn't misreport success when clipboard access fails.src/main/index.ts-140-140 (1)
140-140:⚠️ Potential issue | 🟡 MinorFix
outputPathsuffix replacement to avoidRegExp+ case-sensitivity issues
extensionOf()lowercases the extension, butrelativePathpreserves the original filename casing; usingrelativePath.replace(new RegExp(\${extension}$ `), '.html')is case-sensitive, so files likenote.MDcan produce anoutputPathending in.MDinstead of.html. Derive the output suffix via string slicing (noRegExp`) .🔧 Proposed fix
- const outputPath = join(repoRoot, 'exports', source, relativePath.replace(new RegExp(`${extension}$`), '.html')); + const outputRelativePath = extension + ? `${relativePath.slice(0, -extension.length)}.html` + : `${relativePath}.html`; + const outputPath = join(repoRoot, 'exports', source, outputRelativePath);🤖 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/main/index.ts` at line 140, The outputPath construction uses a case-sensitive RegExp to strip the extension which fails for differently-cased filenames (e.g. note.MD); instead derive the new suffix by slicing off extension.length from relativePath and appending '.html'. Replace the RegExp replace in the outputPath expression with something like: compute base = relativePath.slice(0, -extension.length) and then join(repoRoot, 'exports', source, base + '.html'); reference the existing variables outputPath, relativePath and the extension returned by extensionOf().Source: Linters/SAST tools
src/renderer/hooks/useGridSelection.ts-59-60 (1)
59-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset selection state when endpoints are outside the grid.
At Line 59, unresolved endpoints return early and keep the previous
selectedCells, so stale selection can remain active after selecting non-grid content.Suggested fix
- if (!start || !end) return; + if (!start || !end) { + setState({ selectedCells: [], selectionRange: null }); + return; + }🤖 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/renderer/hooks/useGridSelection.ts` around lines 59 - 60, When start or end are falsy the current code returns early and leaves stale selectedCells; change the guard in useGridSelection so that when (!start || !end) you clear the selection state (e.g., call setSelectedCells([]) and reset any selection start/end state) before returning. Update the block around the if (!start || !end) check in useGridSelection.ts to explicitly clear selectedCells and related selection state (start/end) to ensure no stale selection remains when endpoints fall outside the grid.src/renderer/components/GridPanel.tsx-31-33 (1)
31-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse actual row count for the “Rows” metric.
Line 32 renders
grid.blockCountunder “Rows”. Usegrid.rows.lengthso the label matches the displayed metric.🐛 Proposed fix
- <dd className="font-mono text-xl text-forge-bright">{grid.blockCount}</dd> + <dd className="font-mono text-xl text-forge-bright">{grid.rows.length}</dd>🤖 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/renderer/components/GridPanel.tsx` around lines 31 - 33, The "Rows" metric in GridPanel.tsx is showing grid.blockCount instead of the actual number of rows; update the rendering in the component to use grid.rows.length (replace grid.blockCount with grid.rows.length in the JSX where the "Rows" <dd> is rendered) so the label matches the displayed metric.
🧹 Nitpick comments (3)
src/node-shims.d.ts (1)
92-94: Fix IPC typing shim: changeipcMain.handlerest args fromnever[]tounknown[]
src/node-shims.d.tstypesipcMain.handle(... listener: (event: IpcMainInvokeEvent, ...args: never[]) => unknown), but the main process registers handlers with real payloads (e.g.documents:loadpassessource: DocumentSourceandrelativePath: string). This shim signature can’t model/propagate argument contracts betweenipcRenderer.invoke(...args)andipcMain.handle, weakening compile-time drift detection.Suggested typing shape
export const ipcMain: { - handle(channel: string, listener: (event: IpcMainInvokeEvent, ...args: never[]) => unknown): void; + handle(channel: string, listener: (event: IpcMainInvokeEvent, ...args: unknown[]) => unknown): void; };🤖 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/node-shims.d.ts` around lines 92 - 94, The ipcMain.handle shim uses an incorrect rest-arg type (...args: never[]) which prevents any invoke payload typings from propagating; update the declaration for ipcMain.handle so the listener signature accepts a variadic unknown[] (e.g., (event: IpcMainInvokeEvent, ...args: unknown[]) => unknown) so real payloads like DocumentSource and string are allowed and TypeScript can track argument contracts; adjust the export const ipcMain type accordingly and run typechecks to ensure existing handlers still compile.src/parser/markdownParser.test.ts (1)
69-81: ⚡ Quick winMake temp test workspace cleanup unconditional and per-test isolated.
Cleanup currently runs only at the end of each test body. If an assertion fails earlier,
.tmp-forge-testsremains and can cascade failures into following cases.🔧 Proposed refactor pattern
-import { test } from 'node:test'; +import { test } from 'node:test'; -test('validation catches unknown block references', () => { - const root = join(process.cwd(), '.tmp-forge-tests'); +test('validation catches unknown block references', (t) => { + const root = join(process.cwd(), '.tmp-forge-tests-validation'); rmSync(root, { recursive: true, force: true }); mkdirSync(root, { recursive: true }); + t.after(() => rmSync(root, { recursive: true, force: true })); // ... body ... - rmSync(root, { recursive: true, force: true }); });Apply the same pattern to the other temp-dir tests in this file.
Also applies to: 84-96, 100-115, 118-130, 132-147
🤖 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/parser/markdownParser.test.ts` around lines 69 - 81, The test currently creates and removes the temp workspace inline so a failed assertion leaves .tmp-forge-tests behind; change this to unconditional per-test setup/teardown by moving creation (mkdirSync(root,...)) into a beforeEach that sets root and markdownPath and moving rmSync(root, { recursive: true, force: true }) into an afterEach (or wrap the test body in try/finally) so cleanup always runs; update the test that writes bad.md (writeFileSync, writeFileSync(join(root,...)), parseMarkdown, validateMarkdownFile, etc.) to rely on the shared per-test root and apply the same beforeEach/afterEach pattern to the other temp-dir tests mentioned (lines 84-96, 100-115, 118-130, 132-147).src/renderer/context/LayerProvider.tsx (1)
23-23: ⚡ Quick winUnify layer-id validation with config to avoid persistence drift.
Line 23 hardcodes valid layer IDs again. If
layerConfigschanges, stored layer state can be silently discarded until this list is manually updated too.♻️ Proposed fix
import { createContext, useContext, useMemo, useState } from 'react'; -import type { LayerId } from '../config/layers.ts'; +import { layerConfigs } from '../config/layers.ts'; +import type { LayerId } from '../config/layers.ts'; @@ function readInitialLayers(): LayerId[] { @@ - return parsed.filter((item): item is LayerId => item === 'claims' || item === 'equations' || item === 'glossary' || item === 'audio'); + const valid = new Set<LayerId>(layerConfigs.map((layer) => layer.id)); + return parsed.filter((item): item is LayerId => typeof item === 'string' && valid.has(item as LayerId)); } catch { return []; } }🤖 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/renderer/context/LayerProvider.tsx` at line 23, The filter is hardcoding valid layer IDs (the inline check in LayerProvider.tsx) which can drift from the canonical layerConfigs; change the validation to derive allowed IDs from the existing layerConfigs (e.g., use Object.keys(layerConfigs) or map layerConfigs to their id values) and replace the inline equality checks in the parsed.filter callback so stored layer state is validated against the live layerConfigs source (refer to the parsed.filter usage and layerConfigs symbol to locate and update the code).
🤖 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 `@package.json`:
- Around line 10-12: Add a strict Node engine range to package.json to guarantee
compatibility with the experimental --experimental-transform-types flag used by
the "forge" and "test" npm scripts: update the package.json to include an
"engines" field that specifies Node >=22.7.0 and excludes Node 26+ (for example
"node": ">=22.7.0 <26.0.0") so CI and local runs fail fast on unsupported Node
versions; ensure the engine range is published and CI respects it (e.g.,
npm/yarn engine-strict or setup action) so the "forge" and "test" commands using
--experimental-transform-types run only on supported Node releases.
In `@src/engine/engine.ts`:
- Around line 82-99: resolveEnginesRoot currently falls back to
resolve('_engines'), which uses the process CWD; change the fallback to be
vault-local by returning resolve(contentFolder, '_engines') instead so
resolveEnginesRoot(contentFolder, enginesFolder) always prefers the vault path
when no explicit enginesFolder is provided; update the resolve call in
resolveEnginesRoot (and no other changes needed in loadEngines or
defaultEnginesRoot) and keep the existsSync checks as-is.
In `@src/export/htmlExporter.ts`:
- Around line 66-71: The template inserts claim.confidence raw into HTML,
allowing injection; wrap the value with the existing escapeHtml helper before
interpolation (e.g., use escapeHtml(String(claim.confidence ?? 'unset'))), and
ensure you convert non-strings to string first so escapeHtml always receives a
string; update the template where claim.confidence is used alongside
escapeHtml(claim.id) and renderInline to prevent HTML/script injection.
In `@src/layers/layerStore.ts`:
- Around line 11-14: The readOptionalJson function currently calls JSON.parse on
untrusted sidecar files and can throw; wrap the parse in a try/catch inside
readOptionalJson so that on any parse error it returns undefined (or an explicit
error sentinel) instead of propagating an exception, preserving the existing
return type and logging a debug/error as needed; similarly, in the validation
logic (the validate/validation flow that parses sidecar content around the lines
indicated in validate.ts) wrap JSON.parse in a try/catch and convert parse
failures into ValidationMessage entries with severity 'fail' (emit a
ValidationMessage describing the malformed sidecar and include any parse error
message) instead of allowing the exception to abort the validation flow.
In `@src/main/index.ts`:
- Around line 110-115: Validate the file extension against the allowlist before
performing any file I/O: in loadDocument, call extensionOf(relativePath) and
check it against the allowedExtensions set (or isAllowedExtension) before
calling safeJoin/readFile or nodeFor; if the extension is not allowed, throw or
return an error immediately. Apply the same pre-I/O extension validation to the
export handler(s) referenced around lines 135-140 (the export function handling
document paths) so no read/write occurs for disallowed extensions.
- Around line 54-59: safeJoin's startsWith check is unsafe—replace its check
with a robust boundary test using relative(resolvedRoot, fullPath) and ensure it
doesn't begin with '..' (update function safeJoin). In exportDocument,
canonicalize and validate relativePath against the intended exports directory
(use resolved export root + safeJoin) and sanitize output filename using
path.extname to replace the extension deterministically rather than
relativePath.replace with a user-controlled regex; also ensure the final write
path remains inside the exports root via safeJoin check. Enforce the
documentExtensions allowlist for IPC entry points "documents:load" and
"documents:export" (same validation used by walkDocumentTree/documentExtensions)
before reading or exporting files. Finally, remove any construction of regex
from untrusted extension strings—use extname or exact suffix checks when
computing the .html output filename (references: safeJoin, exportDocument,
documents:load, documents:export, walkDocumentTree, documentExtensions,
relativePath.replace).
In `@src/mirror/mirror.ts`:
- Around line 42-64: The function findMirrorContentFiles is currently traversing
a user-specified mirror directory because it only checks
shouldSkipMirrorDirectory by name; fix it by skipping the actual resolved mirror
root path during recursion: inside the for loop where entry.isDirectory() is
checked, compute the resolved entryPath and compare it to the resolved
mirrorRoot (the mirrorRoot parameter) and continue (skip) when they are equal;
keep the existing shouldSkipMirrorDirectory(entry.name) check as-is so both
name-based and path-based skipping occur.
In `@src/parser/blockId.ts`:
- Around line 36-49: createBlockId currently generates a repairedId by appending
a sequence based only on the per-base count, which can still collide with IDs
previously emitted for other bases; change createBlockId to deterministically
loop to find a globally unused candidate by deriving base (explicitId ??
`${type}-${slugify(text)}`), then while usedIds.has(candidate) increment a
counter and compute candidate (e.g., `${base}-${counter.padStart(3,'0')}`),
finally set usedIds.set(candidate, 1) and return id: candidate with collision:{
requestedId: base, repairedId: candidate } so the returned repairedId is
guaranteed unique across usedIds.
In `@src/parser/markdownParser.test.ts`:
- Around line 128-129: The test assertion for plan.files[0].mirrorDirectory is
not portable on Windows because it expects POSIX separators; update the test in
markdownParser.test.ts to compare against a platform-joined path (e.g. use
path.join('folder','note')) or normalize both sides (path.normalize) before
calling endsWith, and import Node's path module if not already present; target
the assertion referencing plan.files[0].mirrorDirectory and replace the
hardcoded 'folder/note' check with a path.join/path.normalize-based comparison.
In `@src/parser/stamper.ts`:
- Around line 67-69: The current backupPath (`${markdownPath}.bak`) in
stamper.ts overwrites previous backups; change the backup creation logic before
copyFileSync(markdownPath, backupPath) to generate a non-destructive unique name
(e.g., append a timestamp or incrementing suffix like `.bak`, `.bak.1`, `.bak.2`
until a free filename is found) and then call copyFileSync using that unique
backupPath; ensure markdownPath and writeFileSync(markdownPath, result.output,
'utf8') remain unchanged except for using the newly computed backupPath.
In `@src/renderer/App.tsx`:
- Around line 71-79: The effect calling window.forge.listDocuments lacks
rejection handling and may leave a stale loadedDocument visible on failure;
update the useEffect (and similar async IPC callers) to catch promise rejections
from window.forge.listDocuments, clear or reset loadedDocument/state (e.g., call
setSelectedDocument(null) or setLoadedDocument(undefined) before/when an IPC
error occurs), and ensure you still respect the isMounted guard before setting
state; do the same pattern for the other async calls in this file (the calls
around lines referencing setTree, pickInitialDocument, and any loadedDocument
setters) so errors are logged/handled and stale UI is cleared.
In `@src/renderer/components/ArticleViewer.tsx`:
- Line 22: The ArticleViewer component currently injects loaded HTML via
dangerouslySetInnerHTML (see ArticleViewer.tsx: the div rendering children ??
<div dangerouslySetInnerHTML={{ __html: html ?? '' }} />), which must be
sanitized first; add an allowlist-based sanitizer dependency (e.g., DOMPurify)
to the project, import it into ArticleViewer, call
DOMPurify.sanitize(loadedDocument.html) (or sanitize html before passing into
the component) and use the sanitized string in dangerouslySetInnerHTML instead
of the raw html; ensure the sanitizer is configured to disallow scripts, event
handlers, and dangerous protocols so the rendered output cannot call
window.forge or other IPC APIs.
In `@src/renderer/components/GridOverlay.tsx`:
- Around line 25-29: shouldSkipNode currently lets UI text (headers/meta/tabs)
through, so text traversal in .article-content picks up non-block text and
misaligns grid wrapping; update shouldSkipNode (used by the traversal at the
.article-content loop) to only allow nodes that are inside real block content by
adding a positive containment check like node.closest('.block-content,
.forge-block') and return true if that check fails, while keeping the existing
exclusion of script/style/nav/footer/button/select and forge-specific UI
selectors; ensure the traversal (lines ~98–113) uses this updated shouldSkipNode
to skip non-block UI text.
In `@src/renderer/components/LayerToggleBar.tsx`:
- Around line 13-15: The code is calling the hook useLayer inside a for loop
which breaks the Rules of Hooks; move the hook call into a component body that
is rendered per config (e.g., create/ensure a LayerRuntime or LayerItem
component that calls useLayer({ config, active: isLayerActive(config.id),
articleVersion }) in its top-level body) and replace the for loop with
layerConfigs.map(cfg => <LayerRuntime key={cfg.id} config={cfg} ... />) so hooks
run in a stable, deterministic order; also ensure you pass a stable key
(config.id) and do not conditionally call useLayer inside that component.
In `@src/renderer/hooks/useLayer.ts`:
- Around line 104-106: The Promise returned by injectLayer(config) is not
handling rejection, causing unhandled promise rejections from internals like
readSharedAsset(); update the call site that currently does `void
injectLayer(config).then(() => { if (cancelled) cleanupLayer(config); });` to
append a .catch handler that logs the error (use your existing logger) and
ensures cleanupLayer(config) is called when necessary (respecting the cancelled
flag) so failures are cleaned up and errors are surfaced; reference injectLayer,
cleanupLayer, and the cancelled variable when making the change.
In `@src/vault/importer.ts`:
- Around line 56-60: Before calling findImportableFiles, detect if vaultRoot is
nested inside sourceRoot (e.g., compute relative = path.relative(sourceRoot,
vaultRoot) and check that relative is not '' and does not start with '..') and
fail fast or exclude that subtree; update the importer logic around
sourceRoot/vaultRoot (the block that calls resolve, statSync, and
findImportableFiles) to throw a clear Error when vaultRoot is inside sourceRoot
(or alternatively pass an exclusion for vaultRoot into findImportableFiles) so
the traversal cannot re-import the vault's own files.
---
Minor comments:
In `@exports/article.html`:
- Around line 148-153: Replace the workspace-specific absolute path and exact
export timestamp in exports/article.html: update the <dd> value for the "Source"
entry (currently "/workspace/Forge-electron/samples/article.md") to a redacted
or relative form like "samples/article.md" or "<redacted>/samples/article.md",
and change the <dd> value for "Export timestamp" (currently
"2026-06-11T12:23:29.520Z") to a stable placeholder such as "EXPORT_TIMESTAMP"
or a fixed ISO stub ("1970-01-01T00:00:00Z") so the fixture no longer leaks
local filesystem details or churns on regeneration; ensure only the text nodes
for those <dd> entries are modified and leave "Document ID", "Block count",
"Layer file", and "Claims file" entries unchanged.
In `@README.md`:
- Line 41: Update the sentence that starts "The desktop shell is scaffolded with
Electron + electron-vite + React + TypeScript." to a tighter, more professional
phrasing: replace it with something like "The desktop shell is built with
Electron, electron-vite, React, and TypeScript; it launches a frameless, dark
FORGE window with a custom topbar, sidebar article switcher, content area, and
footer." Locate and edit the sentence (the line beginning "The desktop shell is
scaffolded...") in README.md.
In `@src/cli/forge.ts`:
- Around line 37-41: The optionValue function currently treats the next token as
a value even if it's another flag; update optionValue(flags: string[], name:
string) to verify that index+1 exists and that flags[index + 1] is a real value
(not empty and does not start with '-') and return undefined otherwise; modify
the lookup in optionValue (and any callers that rely on it, e.g., parsing for
--vault / --mirror) so omitted values are treated as missing rather than using
the next flag token.
In `@src/export/htmlExporter.ts`:
- Around line 251-258: Wrap the clipboard write in a try/catch inside the
button.click handler that uses the existing dataset.copyTarget and URL-building
logic; await navigator.clipboard.writeText(url.toString()) inside the try and
only set button.textContent = 'Copied' on success, while in the catch set a
failure state (e.g., 'Copy failed' or a permission message), log the error
(console.error or process logger), and restore the original 'Copy link' text
after the timeout so the UI doesn't misreport success when clipboard access
fails.
In `@src/main/index.ts`:
- Line 140: The outputPath construction uses a case-sensitive RegExp to strip
the extension which fails for differently-cased filenames (e.g. note.MD);
instead derive the new suffix by slicing off extension.length from relativePath
and appending '.html'. Replace the RegExp replace in the outputPath expression
with something like: compute base = relativePath.slice(0, -extension.length) and
then join(repoRoot, 'exports', source, base + '.html'); reference the existing
variables outputPath, relativePath and the extension returned by extensionOf().
In `@src/renderer/components/GridPanel.tsx`:
- Around line 31-33: The "Rows" metric in GridPanel.tsx is showing
grid.blockCount instead of the actual number of rows; update the rendering in
the component to use grid.rows.length (replace grid.blockCount with
grid.rows.length in the JSX where the "Rows" <dd> is rendered) so the label
matches the displayed metric.
In `@src/renderer/hooks/useGridSelection.ts`:
- Around line 59-60: When start or end are falsy the current code returns early
and leaves stale selectedCells; change the guard in useGridSelection so that
when (!start || !end) you clear the selection state (e.g., call
setSelectedCells([]) and reset any selection start/end state) before returning.
Update the block around the if (!start || !end) check in useGridSelection.ts to
explicitly clear selectedCells and related selection state (start/end) to ensure
no stale selection remains when endpoints fall outside the grid.
---
Nitpick comments:
In `@src/node-shims.d.ts`:
- Around line 92-94: The ipcMain.handle shim uses an incorrect rest-arg type
(...args: never[]) which prevents any invoke payload typings from propagating;
update the declaration for ipcMain.handle so the listener signature accepts a
variadic unknown[] (e.g., (event: IpcMainInvokeEvent, ...args: unknown[]) =>
unknown) so real payloads like DocumentSource and string are allowed and
TypeScript can track argument contracts; adjust the export const ipcMain type
accordingly and run typechecks to ensure existing handlers still compile.
In `@src/parser/markdownParser.test.ts`:
- Around line 69-81: The test currently creates and removes the temp workspace
inline so a failed assertion leaves .tmp-forge-tests behind; change this to
unconditional per-test setup/teardown by moving creation (mkdirSync(root,...))
into a beforeEach that sets root and markdownPath and moving rmSync(root, {
recursive: true, force: true }) into an afterEach (or wrap the test body in
try/finally) so cleanup always runs; update the test that writes bad.md
(writeFileSync, writeFileSync(join(root,...)), parseMarkdown,
validateMarkdownFile, etc.) to rely on the shared per-test root and apply the
same beforeEach/afterEach pattern to the other temp-dir tests mentioned (lines
84-96, 100-115, 118-130, 132-147).
In `@src/renderer/context/LayerProvider.tsx`:
- Line 23: The filter is hardcoding valid layer IDs (the inline check in
LayerProvider.tsx) which can drift from the canonical layerConfigs; change the
validation to derive allowed IDs from the existing layerConfigs (e.g., use
Object.keys(layerConfigs) or map layerConfigs to their id values) and replace
the inline equality checks in the parsed.filter callback so stored layer state
is validated against the live layerConfigs source (refer to the parsed.filter
usage and layerConfigs symbol to locate and update the code).
🪄 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: bd856571-7cde-45ff-860a-e6b1af5d50d0
📒 Files selected for processing (51)
README.md_engines/export_html.yaml_engines/statistics.yamldocs/DATA_MIRROR_AND_GLOBAL_ENGINE.mddocs/PHASE_2_MARKDOWN_HARDENING.mddocs/VAULT_IMPORT_AND_COLUMNS.mdelectron.vite.config.tsexports/article.htmlindex.htmlpackage.jsonpostcss.config.cjssamples/article.claims.jsonsamples/article.forge.jsonsamples/article.layers.jsonsamples/article.mdsrc/cli/forge.tssrc/engine/engine.tssrc/export/htmlExporter.tssrc/layers/layerStore.tssrc/layers/layerTypes.tssrc/main/index.tssrc/mirror/mirror.tssrc/node-shims.d.tssrc/parser/blockId.tssrc/parser/markdownParser.test.tssrc/parser/markdownParser.tssrc/parser/stamper.tssrc/preload/index.tssrc/renderer/App.tsxsrc/renderer/components/ArticleViewer.tsxsrc/renderer/components/FileTree.tsxsrc/renderer/components/ForgeDocumentView.tsxsrc/renderer/components/GridOverlay.tsxsrc/renderer/components/GridPanel.tsxsrc/renderer/components/LayerToggleBar.tsxsrc/renderer/components/RightSidebar.tsxsrc/renderer/components/Sidebar.tsxsrc/renderer/components/TitleBar.tsxsrc/renderer/config/layers.tssrc/renderer/context/LayerProvider.tsxsrc/renderer/env.d.tssrc/renderer/hooks/useGrid.tssrc/renderer/hooks/useGridSelection.tssrc/renderer/hooks/useLayer.tssrc/renderer/lib/grid.tssrc/renderer/main.tsxsrc/renderer/styles/index.csssrc/validation/validate.tssrc/vault/importer.tstailwind.config.cjstsconfig.json
Owner
Author
|
Good |
1 similar comment
Owner
Author
|
Good |
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.
Motivation
_data/mirrors.Description
src/cli/forge.tswith commandsparse,stamp,validate,export,export-folder,import-folder,mirror,engines, andrun-engine, plus samplepackage.jsonscripts and TypeScript config.src/parser/*(parser, block ID generator, stamper),src/validation/validate.ts, sidecar layer/claims loadersrc/layers/*, vault importersrc/vault/importer.ts, and mirror/plan writersrc/mirror/mirror.ts.src/engine/engine.tswith built-inexport_htmlandstatisticsengines plus YAML defaults in_engines/.src/export/htmlExporter.tsthat renders TOC, block anchors, layer tabs, claims, metadata, and sliding side columns; add a sample export atexports/article.htmland sample Markdown/sidecars undersamples/.src/main,src/preload,src/renderer/*components, grid subsystemsrc/renderer/lib/grid.ts, layer runtime hooks, and supporting config/asset files (index.html,electron.vite.config.ts,tailwind.config.cjs,postcss.config.cjs).Testing
src/parser/markdownParser.test.tsthat exercise block ID stability, duplicate handling, parsing of headings/blocks/code/math, stamping/sidecar validation, vault import, mirror sync, engine dry-run, and the grid API; tests are runnable withnpm test.npm run typecheckto validate the TypeScript surface and module declarations.npm testand typecheck vianpm run typecheck, and both completed successfully.Codex Task
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests