feat: dispatch document editors through a registry#3932
Open
Crash-- wants to merge 8 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a centralized editor registry in Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
zatteo
reviewed
Jun 18, 2026
zatteo
reviewed
Jun 18, 2026
Member
|
Very very promising refactoring I can't wait to see a new editor added in 10 lines 🎉 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
BundleMonFiles updated (2)
Unchanged files (18)
Total files change +15B 0% Groups updated (1)
Unchanged groups (2)
Final result: ✅ View report in BundleMon website ➡️ |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Crash--
force-pushed
the
feat/editor-registry
branch
from
June 21, 2026 05:47
0e2c7ba to
3cd5049
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Crash--
marked this pull request as ready for review
June 21, 2026 06:01
Crash--
requested review from
JF-Cozy,
doubleface,
lethemanh and
rezk2ll
as code owners
June 21, 2026 06:01
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/modules/views/editor/routes.jsx (1)
29-31: ⚡ Quick winGuard missing route factories before invocation.
Line 31 directly calls
ROUTE_FACTORIES[slug](). If a new editor is added toEDITORSwithout updating this map, routing crashes at runtime. Add an explicit invariant (or safe skip) here.Proposed change
export const getEditorRoutes = () => ( <> - {EDITORS.map(({ slug, flag: editorFlag }) => - editorFlag === null || flag(editorFlag) ? ( - <React.Fragment key={slug}>{ROUTE_FACTORIES[slug]()}</React.Fragment> - ) : null - )} + {EDITORS.map(({ slug, flag: editorFlag }) => { + const shouldMount = editorFlag === null || flag(editorFlag) + if (!shouldMount) return null + + const buildRoutes = ROUTE_FACTORIES[slug] + if (!buildRoutes) { + throw new Error(`Missing route factory for editor slug "${slug}"`) + } + + return <React.Fragment key={slug}>{buildRoutes()}</React.Fragment> + })} </> )🤖 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/modules/views/editor/routes.jsx` around lines 29 - 31, The code in the EDITORS map directly invokes ROUTE_FACTORIES[slug]() without verifying that the factory exists in the ROUTE_FACTORIES map. If a slug is added to EDITORS without a corresponding entry in ROUTE_FACTORIES, this will cause a runtime crash when attempting to call undefined as a function. Add a guard condition before invoking ROUTE_FACTORIES[slug]() to check that the factory exists, either by using an invariant assertion or by conditionally rendering the route only when ROUTE_FACTORIES[slug] is defined, ensuring all editors in EDITORS have matching factories before their routes are created.
🤖 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 `@src/modules/views/editor/registry.js`:
- Around line 88-94: The `findEditorForFile` function unconditionally invokes
`editor.isEnabled(context)` on line 92, but the EditorDescriptor documentation
indicates that `isEnabled` is optional. Add a guard check to verify that
`editor.isEnabled` exists and is a function before calling it, similar to the
existing guard for `editor.matchesFile`. This same issue also exists in another
resolver around line 108 that should be fixed identically.
---
Nitpick comments:
In `@src/modules/views/editor/routes.jsx`:
- Around line 29-31: The code in the EDITORS map directly invokes
ROUTE_FACTORIES[slug]() without verifying that the factory exists in the
ROUTE_FACTORIES map. If a slug is added to EDITORS without a corresponding entry
in ROUTE_FACTORIES, this will cause a runtime crash when attempting to call
undefined as a function. Add a guard condition before invoking
ROUTE_FACTORIES[slug]() to check that the factory exists, either by using an
invariant assertion or by conditionally rendering the route only when
ROUTE_FACTORIES[slug] is defined, ensuring all editors in EDITORS have matching
factories before their routes are created.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bbc2475-ba51-4f70-8ec0-a55d9d7df49d
📒 Files selected for processing (10)
src/modules/navigation/AppRoute.jsxsrc/modules/navigation/hooks/helpers.spec.jssrc/modules/navigation/hooks/helpers.tssrc/modules/navigation/hooks/useFileLink.tsxsrc/modules/views/SharedDrive/FilesViewerSharedDriveRootFile.jsxsrc/modules/views/SharedDrive/FilesViewerSharedDriveRootFile.spec.jsxsrc/modules/views/editor/registry.jssrc/modules/views/editor/registry.spec.jssrc/modules/views/editor/routes.jsxsrc/modules/views/editor/routes.spec.jsx
Comment on lines
+88
to
+94
| export const findEditorForFile = (file, context = {}) => | ||
| EDITORS.find( | ||
| editor => | ||
| Boolean(editor.matchesFile) && | ||
| editor.isEnabled(context) && | ||
| editor.matchesFile(file) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Guard optional isEnabled before invocation (Line 92 and Line 108).
EditorDescriptor.isEnabled is documented as optional, but both resolvers call it unconditionally. A descriptor with matchesFile/matchesShortcutTarget and no isEnabled will throw at runtime.
Proposed fix
export const findEditorForFile = (file, context = {}) =>
EDITORS.find(
editor =>
Boolean(editor.matchesFile) &&
- editor.isEnabled(context) &&
+ (editor.isEnabled?.(context) ?? true) &&
editor.matchesFile(file)
)
export const findEditorForShortcutTarget = (target, context = {}) =>
EDITORS.find(
editor =>
Boolean(editor.matchesShortcutTarget) &&
- editor.isEnabled(context) &&
+ (editor.isEnabled?.(context) ?? true) &&
editor.matchesShortcutTarget(target)
)Also applies to: 104-110
🤖 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/modules/views/editor/registry.js` around lines 88 - 94, The
`findEditorForFile` function unconditionally invokes `editor.isEnabled(context)`
on line 92, but the EditorDescriptor documentation indicates that `isEnabled` is
optional. Add a guard check to verify that `editor.isEnabled` exists and is a
function before calling it, similar to the existing guard for
`editor.matchesFile`. This same issue also exists in another resolver around
line 108 that should be fixed identically.
Crash--
force-pushed
the
feat/editor-registry
branch
from
June 22, 2026 08:33
3cd5049 to
a317e4c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/modules/views/SharedDrive/FilesViewerSharedDriveRootFile.spec.jsx (1)
91-126: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen redirect tests by asserting resolver call contract and no-navigation fallback
At Line 91 and Line 111, consider asserting
findEditorForFileis called with the resolved file and{ isDesktop: true }, and in the bridge case assert nonavigate:marker is rendered. This locks in the breakpoint-aware dispatch contract and prevents false positives if redirect logic regresses.Suggested test hardening
it('redirects an editor document to its editor', () => { + const fetchedFile = { + _id: 'canonical-id', + id: 'canonical-id', + name: 'Drawing.excalidraw' + } mockFindEditorForFile.mockReturnValue({ slug: 'excalidraw', kind: 'editor', makeRoute: file => `/excalidraw/drive-1/${file._id}` }) renderRootFileViewer({ - fetchedFile: { - _id: 'canonical-id', - id: 'canonical-id', - name: 'Drawing.excalidraw' - } + fetchedFile }) + expect(mockFindEditorForFile).toHaveBeenCalledWith(fetchedFile, { + isDesktop: true + }) expect( screen.getByText('navigate:/excalidraw/drive-1/canonical-id') ).toBeInTheDocument() expect(mockFilesViewer).not.toHaveBeenCalled() }) it('does not redirect a bridge document (it has no in-app route)', () => { + const fetchedFile = { + _id: 'canonical-id', + id: 'canonical-id', + name: 'Budget.grist' + } mockFindEditorForFile.mockReturnValue({ slug: 'grist', kind: 'bridge', makeRoute: file => `/bridge/grist/${file.metadata.externalId}` }) renderRootFileViewer({ - fetchedFile: { - _id: 'canonical-id', - id: 'canonical-id', - name: 'Budget.grist' - } + fetchedFile }) + expect(mockFindEditorForFile).toHaveBeenCalledWith(fetchedFile, { + isDesktop: true + }) + expect(screen.queryByText(/^navigate:/)).not.toBeInTheDocument() expect(screen.getByText('files-viewer')).toBeInTheDocument() })🤖 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/modules/views/SharedDrive/FilesViewerSharedDriveRootFile.spec.jsx` around lines 91 - 126, The two tests need additional assertions to strengthen the contract verification. In the first test that redirects an editor document (starting with "redirects an editor document to its editor"), add an assertion that mockFindEditorForFile was called with the fetchedFile object and an options object containing isDesktop: true to verify the dispatch contract. In the second test that handles bridge documents (starting with "does not redirect a bridge document"), add an assertion that verifies no navigate: marker is rendered to ensure the fallback behavior is correct and prevent regressions if redirect logic changes.src/modules/navigation/hooks/helpers.spec.js (1)
14-22: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider consistent mock patterns across helper modules.
The Excalidraw mock uses
jest.requireActualto preserve unmodified exports while the OnlyOffice mock replaces the entire module. This inconsistency could cause subtle issues if the OnlyOffice helpers module exports additional functions (likeshouldBeOpenedByOnlyOffice) that the registry uses.♻️ Suggested consistent pattern
jest.mock('modules/views/OnlyOffice/helpers', () => ({ + ...jest.requireActual('modules/views/OnlyOffice/helpers'), makeOnlyOfficeFileRoute: jest.fn(), isOfficeEnabled: jest.fn() }))🤖 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/modules/navigation/hooks/helpers.spec.js` around lines 14 - 22, The OnlyOffice helpers mock is replacing the entire module without preserving actual exports, while the Excalidraw helpers mock uses jest.requireActual to preserve unmodified exports. Update the jest.mock call for 'modules/views/OnlyOffice/helpers' to use the same pattern as the Excalidraw mock by adding jest.requireActual with the spread operator at the beginning of the mock object. This ensures all actual exports (including any additional functions like shouldBeOpenedByOnlyOffice) are available in the mocked module while only the specified functions (makeOnlyOfficeFileRoute and isOfficeEnabled) are overridden with jest.fn().
🤖 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.
Nitpick comments:
In `@src/modules/navigation/hooks/helpers.spec.js`:
- Around line 14-22: The OnlyOffice helpers mock is replacing the entire module
without preserving actual exports, while the Excalidraw helpers mock uses
jest.requireActual to preserve unmodified exports. Update the jest.mock call for
'modules/views/OnlyOffice/helpers' to use the same pattern as the Excalidraw
mock by adding jest.requireActual with the spread operator at the beginning of
the mock object. This ensures all actual exports (including any additional
functions like shouldBeOpenedByOnlyOffice) are available in the mocked module
while only the specified functions (makeOnlyOfficeFileRoute and isOfficeEnabled)
are overridden with jest.fn().
In `@src/modules/views/SharedDrive/FilesViewerSharedDriveRootFile.spec.jsx`:
- Around line 91-126: The two tests need additional assertions to strengthen the
contract verification. In the first test that redirects an editor document
(starting with "redirects an editor document to its editor"), add an assertion
that mockFindEditorForFile was called with the fetchedFile object and an options
object containing isDesktop: true to verify the dispatch contract. In the second
test that handles bridge documents (starting with "does not redirect a bridge
document"), add an assertion that verifies no navigate: marker is rendered to
ensure the fallback behavior is correct and prevent regressions if redirect
logic changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58e829cb-e4c8-4700-b779-d6e5d764acbc
📒 Files selected for processing (12)
src/modules/drive/Toolbar/components/CreateDocumentItems.jsxsrc/modules/drive/Toolbar/components/CreateDocumentItems.spec.jsxsrc/modules/navigation/AppRoute.jsxsrc/modules/navigation/hooks/helpers.spec.jssrc/modules/navigation/hooks/helpers.tssrc/modules/navigation/hooks/useFileLink.tsxsrc/modules/views/SharedDrive/FilesViewerSharedDriveRootFile.jsxsrc/modules/views/SharedDrive/FilesViewerSharedDriveRootFile.spec.jsxsrc/modules/views/editor/registry.jssrc/modules/views/editor/registry.spec.jssrc/modules/views/editor/routes.jsxsrc/modules/views/editor/routes.spec.jsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/modules/views/editor/routes.spec.jsx
- src/modules/views/SharedDrive/FilesViewerSharedDriveRootFile.jsx
- src/modules/views/editor/routes.jsx
- src/modules/views/editor/registry.js
- src/modules/navigation/AppRoute.jsx
- src/modules/navigation/hooks/useFileLink.tsx
Crash--
force-pushed
the
feat/editor-registry
branch
from
June 22, 2026 09:00
a317e4c to
28f7db3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/modules/views/editor/registry.spec.js (1)
180-194: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert the exact feature-flag keys in Docs/Grist create tests.
These tests currently validate only boolean outcomes; they won’t catch a typo/regression in the flag name.
Suggested test hardening
it('offers Docs only privately and behind its flag', () => { mockFlag.mockReturnValue(true) expect(canCreate('docs', { isPublic: false })).toBe(true) + expect(mockFlag).toHaveBeenCalledWith('drive.lasuitedocs.enabled') expect(canCreate('docs', { isPublic: true })).toBe(false) mockFlag.mockReturnValue(false) expect(canCreate('docs', { isPublic: false })).toBe(false) }) it('offers Grist only privately and behind its flag', () => { mockFlag.mockReturnValue(true) expect(canCreate('grist', { isPublic: false })).toBe(true) + expect(mockFlag).toHaveBeenCalledWith('drive.grist.enabled') expect(canCreate('grist', { isPublic: true })).toBe(false) mockFlag.mockReturnValue(false) expect(canCreate('grist', { isPublic: false })).toBe(false) })🤖 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/modules/views/editor/registry.spec.js` around lines 180 - 194, In both test cases "offers Docs only privately and behind its flag" and "offers Grist only privately and behind its flag", add assertions to verify that mockFlag is being called with the exact correct feature-flag key names. For the docs test, verify mockFlag is called with the docs flag key; for the grist test, verify mockFlag is called with the grist flag key. This will catch any typos or regressions in the flag names instead of only validating the boolean outcomes of canCreate.
🤖 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.
Nitpick comments:
In `@src/modules/views/editor/registry.spec.js`:
- Around line 180-194: In both test cases "offers Docs only privately and behind
its flag" and "offers Grist only privately and behind its flag", add assertions
to verify that mockFlag is being called with the exact correct feature-flag key
names. For the docs test, verify mockFlag is called with the docs flag key; for
the grist test, verify mockFlag is called with the grist flag key. This will
catch any typos or regressions in the flag names instead of only validating the
boolean outcomes of canCreate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df145f21-7a90-4856-a0e9-c6e074315cef
📒 Files selected for processing (4)
src/modules/drive/Toolbar/components/CreateDocumentItems.jsxsrc/modules/drive/Toolbar/components/CreateDocumentItems.spec.jsxsrc/modules/views/editor/registry.jssrc/modules/views/editor/registry.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/drive/Toolbar/components/CreateDocumentItems.jsx
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Crash--
force-pushed
the
feat/editor-registry
branch
from
June 26, 2026 15:26
f993c4a to
dcfaf0e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Editor dispatch (Excalidraw, OnlyOffice) was duplicated across computeFileType and computePath. Introduce a single editor registry as the source of truth so a new editor type is wired in one place. As a result, an Excalidraw file shared as a shared-drive root, which the recipient sees as a .url shortcut, now resolves to the Excalidraw editor instead of the generic shared-drive root-file viewer.
A file shared as a shared-drive root is materialized on the recipient as a .url shortcut, and direct links or reloads land on the generic root-file viewer without going through list-level dispatch. That viewer has no inline Excalidraw viewer, so a shared drawing never opened. Once the real file is resolved, redirect documents that match an editor in the registry to their editor.
Editor routes were mounted in two places in AppRoute (OnlyOffice unconditionally, Excalidraw and PDF behind their flags). Move the list of editors and their flag gating into a single registry so AppRoute mounts them with one call and a new editor is added in one place. No behavior change: the same routes are mounted under the same flags.
Each editor descriptor now owns its enablement: isEnabled reads cozy-flags directly instead of receiving precomputed booleans, and the route layer derives slug and flag from the same EDITORS list. computeFileType takes isDesktop (forwarded to the device-aware OnlyOffice check) rather than the enablement booleans. PDF is listed as a route-only editor. No behavior change: enablement resolves through the same helpers as before.
Fold the Docs and Grist handles into the editor registry so file-type,
app and path dispatch read from one list instead of dedicated branches
in computeFileType/computeApp/computePath. Each descriptor declares its
kind ('editor' for in-app editors, 'bridge' for documents opened through
/bridge/<app>) and bridge documents build their own route, so the route
layer mounts nothing for them and the shared-drive root-file viewer never
redirects to a non-existent in-app route.
…stry Move the per-editor add-menu gating into the registry as `create.isAvailable`, next to each editor's dispatch definition, and have CreateDocumentItems iterate the registry: the registry owns both which editors offer a create entry and when it is shown, while the toolbar keeps only the slug-keyed rendering. The React components stay out of the registry (imported by the pure dispatch helpers on the file-listing hot path) for the same reason the route factories do. Note is rendered on its own since it is not a registry editor. Reorder the registry so its order matches the add menu (Docs, Excalidraw, Grist, OnlyOffice), which leaves dispatch unchanged since those matchers are mutually exclusive.
The registry's JSDoc typedefs become real interfaces (EditorDescriptor, EditorContext, CreateMenuContext, MakeRouteOptions, ShortcutTarget), so the editor contract is checked rather than documented. routes.tsx is typed the same way. The specs stay in JavaScript, like helpers.spec.js for the TypeScript helpers.ts they cover: they gain no typedef removal, only fixture casting.
Crash--
force-pushed
the
feat/editor-registry
branch
from
June 29, 2026 14:42
dcfaf0e to
d3d49a5
Compare
There was a problem hiding this comment.
(3 files improve in Code Health)
Our agent can fix these. Install it.
Gates Passed
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| helpers.ts | 9.11 → 9.18 | Complex Method |
| AppRoute.jsx | 9.22 → 9.23 | Large Method |
| CreateDocumentItems.jsx | 9.69 → 10.00 | Complex Method |
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
zatteo
reviewed
Jun 30, 2026
| /** cozy-flag that gates mounting the editor's routes; `null` mounts them | ||
| * unconditionally (the editor gates access internally). Irrelevant for | ||
| * `'bridge'` documents (they mount no route). */ | ||
| flag: string | null |
Member
There was a problem hiding this comment.
"flag" name is a little misleading: we end with flag: null for editor that "have a flag" literally because it is only to gate editor route (which I understand must be different that isEnabled)
zatteo
approved these changes
Jun 30, 2026
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
2 participants
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
computeFileType,computeAppandcomputePathread from the registry instead of per-editor branches./bridge/<app>) into the registry next to the in-app editors, tagged with akindfield so only in-app editors mount routes and are used as in-app redirect targets, while bridge documents keep their/bridgelink.create.isAvailable, so the registry owns which editors offer a create entry and when it shows; the toolbar keeps only the rendering..urlshortcut) to its editor, and redirect editor documents from the shared-drive root-file viewer so direct links and reloads reach the editor too.Note on the repeated slug keys between
registry.tsand the slug maps inroutes.tsx(ROUTE_FACTORIES) andCreateDocumentItems.jsx(CREATE_ITEM_RENDERERS):registry.tsholds only data and pure dispatch logic with no JSX, because it is imported bycomputeFileType/computePathand runs on every file row of every listing. The slug maps keep the React components out of that hot path. PullingExcalidrawView,OnlyOfficeViewor the create buttons into the registry would bundle them into every listing and make the pure dispatch model depend on the view layer. So the keys repeat on purpose: the registry is the model, and those maps are thin view-layer adapters keyed back to it.Summary by CodeRabbit
Refactor
Tests