feat: support notion style embedded database view #182
Conversation
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
Reviewer's GuideRefactors database view handling to support embedded database blocks with multiple view tabs and updated APIs, while simplifying page and database creation flows and improving error handling and logging. Sequence diagram for creating an embedded linked database from the slash menusequenceDiagram
actor User
participant Editor as Editor_Slate
participant Slash as SlashPanel
participant Ctx as EditorContext
participant API as HTTP_API_Backend
User->>Editor: Type /linked database
Editor->>Slash: Open SlashPanel
Slash->>Ctx: loadViews()
Ctx-->>Slash: views
Slash->>Slash: filter databaseViews
Slash->>Slash: build DatabaseOption list
User->>Slash: Choose database option
Slash->>Ctx: loadViewMeta(databaseViewId)
Ctx->>API: GET /page-view/{databaseViewId}/meta
API-->>Ctx: meta (database_relations)
Ctx-->>Slash: meta
Slash->>Slash: find databaseId via database_relations
Slash->>Ctx: createDatabaseView(documentId, payload)
Ctx->>API: POST /workspace/{ws}/page-view/{documentId}/database-view
API-->>Ctx: CreateDatabaseViewResponse(view_id, database_id)
Ctx-->>Slash: response
Slash->>Slash: createDatabaseNodeData(parentId=documentId, viewIds=[response.view_id], databaseId=response.database_id)
Slash->>Editor: turnInto(blockType, DatabaseNodeData)
Editor-->>User: Embedded database block appears
Updated class diagram for database block, loading strategies, and tabsclassDiagram
class DatabaseNodeData {
<<interface>>
+string view_id
+string[] view_ids
+string parent_id
+string database_id
}
class databaseBlockUtils {
+string[] getViewIds(DatabaseNodeData data)
+string getPrimaryViewId(DatabaseNodeData data)
+bool hasViewIds(DatabaseNodeData data)
+DatabaseNodeData createDatabaseNodeData(string parentId, string[] viewIds, string databaseId)
+DatabaseNodeData addViewId(DatabaseNodeData data, string viewId)
+DatabaseNodeData removeViewId(DatabaseNodeData data, string viewId)
+DatabaseNodeData parseDatabaseNodeData(string jsonString)
+string serializeDatabaseNodeData(DatabaseNodeData data)
}
class DatabaseBlock {
+DatabaseNode node
+render()
+handleNavigateToRow(string rowId)
+handleViewIdsChanged(string[] currentViewIds)
}
class useDatabaseLoading {
+bool notFound
+YDoc doc
+string selectedViewId
+string[] visibleViewIds
+string databaseName
+onChangeView(string viewId)
+loadViewMeta(string viewId, function callback) Promise~View|
}
class DatabaseLoadingConfig {
+string viewId
+string[] allowedViewIds
+function loadView(string viewId) Promise~YDoc|
+function loadViewMeta(string viewId, function callback) Promise~View|
}
class DatabaseLoadingStrategy {
<<interface>>
+bool shouldSetNotFoundOnMetaError()
+bool shouldSkipMetaLoad(string id)
+string[] getVisibleViewIds(View meta)
+string getDatabaseName(View meta)
}
class loadingStrategies {
+DatabaseLoadingStrategy createEmbeddedDatabaseStrategy(DatabaseLoadingConfig config)
+DatabaseLoadingStrategy createStandaloneDatabaseStrategy(DatabaseLoadingConfig config)
+DatabaseLoadingStrategy createLoadingStrategy(DatabaseLoadingConfig config)
+bool isEmbeddedDatabase(string[] allowedViewIds)
}
class DatabaseContextState {
+bool readOnly
+YDoc databaseDoc
+string databasePageId
+string activeViewId
+map~RowId, YDoc~ rowDocMap
+bool isDatabaseRowPage
+string workspaceId
+function createDatabaseView(string viewId, CreateDatabaseViewPayload payload) Promise~CreateDatabaseViewResponse|
}
class Database {
+YDoc doc
+string activeViewId
+string databasePageId
+string databaseName
+string[] visibleViewIds
+onChangeView(string viewId)
+onOpenRowPage(string rowId)
}
class DatabaseViews {
+string activeViewId
+string databasePageId
+string viewName
+string[] visibleViewIds
+onChangeView(string viewId)
+onViewIdsChanged(string[] viewIds)
}
class DatabaseTabs {
+string[] viewIds
+string databasePageId
+string selectedViewId
+setSelectedViewId(string viewId)
+onViewIdsChanged(string[] viewIds)
}
class DatabaseTabItem {
+string viewId
+string databasePageId
+bool deleteDisabled
}
class useAddDatabaseView {
+addDatabaseView(DatabaseViewLayout layout) Promise~string|
}
class SlashPanel {
+string documentId
+loadDatabasesForPicker()
+handleOpenLinkedDatabasePicker(ViewLayout layout, string optionKey)
+handleSelectDatabase(string targetViewId)
}
class EditorContextState {
+function addPage(string parentId, CreatePagePayload payload) Promise~CreatePageResponse|
+function createDatabaseView(string viewId, CreateDatabaseViewPayload payload) Promise~CreateDatabaseViewResponse|
+function loadDatabaseRelations() Promise~DatabaseRelations|
}
DatabaseNodeData <.. databaseBlockUtils : uses
DatabaseBlock ..> DatabaseNodeData : embeds
DatabaseBlock ..> databaseBlockUtils : uses
DatabaseBlock ..> useDatabaseLoading : uses
useDatabaseLoading --> DatabaseLoadingConfig : configured_by
useDatabaseLoading ..> DatabaseLoadingStrategy : uses
loadingStrategies ..> DatabaseLoadingStrategy : implements
loadingStrategies ..> DatabaseLoadingConfig : uses
DatabaseBlock ..> DatabaseContent : renders
DatabaseContent ..> Database : renders
Database ..> DatabaseViews : composes
DatabaseViews ..> DatabaseTabs : composes
DatabaseTabs ..> DatabaseTabItem : renders
Database ..> DatabaseContextState : uses
DatabaseTabs ..> DatabaseContextState : uses
useAddDatabaseView ..> DatabaseContextState : uses
SlashPanel ..> EditorContextState : uses
SlashPanel ..> databaseBlockUtils : createDatabaseNodeData
EditorContextState ..> CreatePageResponse : returns
EditorContextState ..> CreateDatabaseViewResponse : returns
class CreatePageResponse {
+string view_id
+string database_id
}
class CreateDatabaseViewPayload {
+string parent_view_id
+string database_id
+ViewLayout layout
+string name
+bool embedded
}
class CreateDatabaseViewResponse {
+string view_id
+string database_id
}
DatabaseContextState ..> CreateDatabaseViewPayload : uses
DatabaseContextState ..> CreateDatabaseViewResponse : returns
useAddDatabaseView ..> CreateDatabaseViewResponse : uses
DatabaseTabs ..> CreateDatabaseViewPayload : uses
DatabaseBlock ..> DatabaseContextState : casts_context
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
appflowy
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- There are a lot of new
console.debug/console.errorcalls (e.g. inexecuteAPIRequest,useDatabaseLoading,SlashPanel, database view creation) that will run in production; consider gating noisy logs behindimport.meta.env.DEVor a shared logger so normal usage isn’t flooded with debug output. - The new database block utilities (
parseDatabaseNodeData,serializeDatabaseNodeData) don’t appear to be used in this diff; if they’re not needed yet, either remove them or add their first call sites so the intended usage is clear.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are a lot of new `console.debug`/`console.error` calls (e.g. in `executeAPIRequest`, `useDatabaseLoading`, `SlashPanel`, database view creation) that will run in production; consider gating noisy logs behind `import.meta.env.DEV` or a shared logger so normal usage isn’t flooded with debug output.
- The new database block utilities (`parseDatabaseNodeData`, `serializeDatabaseNodeData`) don’t appear to be used in this diff; if they’re not needed yet, either remove them or add their first call sites so the intended usage is clear.
## Individual Comments
### Comment 1
<location> `src/components/ws/useAppflowyWebSocket.ts:150` </location>
<code_context>
setReconnectAttempt(attemptNumber);
// First attempt: random 5-10s delay (thundering herd prevention)
- if (attemptNumber === 1) {
+ if (attemptNumber === 0) {
const firstDelay = 5000 + Math.random() * FIRST_ATTEMPT_MAX_DELAY;
</code_context>
<issue_to_address>
**issue (bug_risk):** Changing the first-attempt check from `=== 1` to `=== 0` may skip the intended thundering-herd backoff.
The reconnect handler now treats `attemptNumber === 0` as the first attempt, but many websocket libraries use `1` as the first reconnect attempt. If `attemptNumber` is 1-based, this condition will never run and the first reconnect won’t get the randomized 5–10s delay for herd prevention. Please confirm the library’s attempt numbering and keep this as `=== 1` (or document the 0-based assumption) if it is 1-based.
</issue_to_address>
### Comment 2
<location> `src/components/editor/components/blocks/database/hooks/useDatabaseLoading.ts:93-95` </location>
<code_context>
+ }
+
if (id === viewId) {
- const meta = await retryLoadViewMeta(viewId, updateVisibleViewIds);
+ try {
+ const meta = await retryLoadViewMeta(viewId, updateVisibleViewIds);
+
+ if (meta) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Meta-loading path invokes `updateVisibleViewIds` twice for the same meta result.
In `loadViewMetaWithCallback`, when `id === viewId` you pass `updateVisibleViewIds` into `retryLoadViewMeta` and also call `await updateVisibleViewIds(meta)` afterward:
```ts
const meta = await retryLoadViewMeta(viewId, updateVisibleViewIds);
...
if (meta) {
await updateVisibleViewIds(meta);
setNotFound(false);
return meta;
}
```
If `retryLoadViewMeta` invokes its callback with the loaded meta, this leads to two calls to `updateVisibleViewIds` for the same result, causing redundant work and extra state updates. Consider relying on just one: either remove the explicit `await updateVisibleViewIds(meta)` or stop passing `updateVisibleViewIds` into `retryLoadViewMeta` and handle it only here.
```suggestion
if (id === viewId) {
try {
const meta = await retryLoadViewMeta(viewId, updateVisibleViewIds);
if (meta) {
setNotFound(false);
return meta;
}
} catch (error) {
```
</issue_to_address>
### Comment 3
<location> `src/components/editor/components/blocks/database/hooks/__tests__/useDatabaseLoading.test.ts:115` </location>
<code_context>
expect(result.current.visibleViewIds).toEqual(['view-1', 'child-1', 'child-2']);
});
- expect(result.current.iidName).toBe('View view-1');
+ expect(result.current.databaseName).toBe('View view-1');
});
</code_context>
<issue_to_address>
**suggestion (testing):** Extend useDatabaseLoading tests to cover the new embedded-database behavior and loading strategy edge cases.
This hook now has significantly more logic (embedded vs standalone behavior, `allowedViewIds`, and different `notFound` semantics), but the test change here only renames the asserted field.
Please add test coverage for:
- Embedded mode (`allowedViewIds`):
- `visibleViewIds` derived from `allowedViewIds` even when meta load fails.
- `selectedViewId` updated when `allowedViewIds` changes and drops the current selection.
- Base-view meta load errors do **not** set `notFound` and the hook still works with the document alone.
- Standalone mode (no `allowedViewIds`):
- `notFound` set on meta load failure.
- `visibleViewIds` from `meta.children` + base `view_id`.
- `shouldSkipMetaLoad` behavior for embedded databases (no meta load for non-base views).
These cases will better exercise the new strategy layer and reduce regression risk in the embedded/standalone logic.
Suggested implementation:
```typescript
expect(result.current.visibleViewIds).toEqual(['view-1', 'child-1', 'child-2']);
});
expect(result.current.databaseName).toBe('View view-1');
});
it('should derive visibleViewIds from allowedViewIds in embedded mode even when meta load fails', async () => {
const allowedViewIds = ['view-1', 'child-1', 'child-2'];
const { result, waitForNextUpdate } = renderHook(() =>
useDatabaseLoading({
// base view is still the same
viewId: 'view-1',
// embedded mode: restrict database to the given views
allowedViewIds,
// simulate that meta loading fails completely
loadMeta: jest.fn().mockRejectedValue(new Error('meta failed')),
// document should still be available and used as the data source
document: {
id: 'database-1',
view_id: 'view-1',
title: 'Embedded DB',
children: [],
} as any,
}),
);
await waitForNextUpdate();
// visibleViewIds is derived purely from allowedViewIds, even though meta failed
expect(result.current.visibleViewIds).toEqual(allowedViewIds);
// embedded mode never considers this a "notFound" just because meta failed
expect(result.current.notFound).toBe(false);
});
it('should update selectedViewId when allowedViewIds changes and drops the current selection', async () => {
const initialAllowedViewIds = ['view-1', 'child-1', 'child-2'];
const nextAllowedViewIds = ['view-1', 'child-2']; // child-1 removed
const wrapperProps = {
viewId: 'child-1',
allowedViewIds: initialAllowedViewIds,
document: {
id: 'database-1',
view_id: 'view-1',
title: 'Embedded DB',
children: [],
} as any,
};
const { result, rerender, waitForNextUpdate } = renderHook(
(props: typeof wrapperProps) =>
useDatabaseLoading({
...props,
loadMeta: jest.fn().mockResolvedValue({
id: 'database-1',
name: 'Embedded DB',
children: [
{ id: 'child-1', name: 'Child 1' },
{ id: 'child-2', name: 'Child 2' },
],
}),
}),
{ initialProps: wrapperProps },
);
await waitForNextUpdate();
// initial selection is the child view
expect(result.current.selectedViewId).toBe('child-1');
// now simulate that the embedding host drops child-1 from allowedViewIds
rerender({
...wrapperProps,
viewId: 'child-1',
allowedViewIds: nextAllowedViewIds,
});
// selectedViewId should be coerced into the first allowed view (base view)
expect(result.current.selectedViewId).toBe('view-1');
expect(result.current.visibleViewIds).toEqual(nextAllowedViewIds);
});
it('should not mark embedded databases as notFound when base-view meta load errors', async () => {
const { result, waitForNextUpdate } = renderHook(() =>
useDatabaseLoading({
viewId: 'view-1',
allowedViewIds: ['view-1'],
// meta load for the base view fails
loadMeta: jest.fn().mockRejectedValue(new Error('meta failed')),
// but the document is still present and should be used
document: {
id: 'database-1',
view_id: 'view-1',
title: 'Embedded DB',
children: [],
} as any,
}),
);
await waitForNextUpdate();
// even though meta failed, embedded databases should still work and not be "notFound"
expect(result.current.notFound).toBe(false);
expect(result.current.visibleViewIds).toEqual(['view-1']);
expect(result.current.databaseName).toBe('Embedded DB');
});
it('should mark standalone databases as notFound when meta load fails', async () => {
const { result, waitForNextUpdate } = renderHook(() =>
useDatabaseLoading({
viewId: 'view-1',
// standalone mode: no allowedViewIds
allowedViewIds: undefined,
// standalone relies on meta; if meta fails we surface notFound
loadMeta: jest.fn().mockRejectedValue(new Error('meta failed')),
document: null,
}),
);
await waitForNextUpdate();
expect(result.current.notFound).toBe(true);
expect(result.current.visibleViewIds).toEqual([]);
});
it('should derive visibleViewIds from meta children and base view in standalone mode', async () => {
const { result, waitForNextUpdate } = renderHook(() =>
useDatabaseLoading({
viewId: 'view-1',
allowedViewIds: undefined,
document: null,
loadMeta: jest.fn().mockResolvedValue({
id: 'database-1',
name: 'Standalone DB',
children: [
{ id: 'child-1', name: 'Child 1' },
{ id: 'child-2', name: 'Child 2' },
],
}),
}),
);
await waitForNextUpdate();
// visibleViewIds include the base view plus meta children
expect(result.current.visibleViewIds).toEqual(['view-1', 'child-1', 'child-2']);
expect(result.current.notFound).toBe(false);
expect(result.current.databaseName).toBe('Standalone DB');
});
it('should skip meta load for non-base views in embedded databases', async () => {
const loadMeta = jest.fn().mockResolvedValue({
id: 'database-1',
name: 'Embedded DB',
children: [
{ id: 'child-1', name: 'Child 1' },
{ id: 'child-2', name: 'Child 2' },
],
});
const { result, rerender, waitForNextUpdate } = renderHook(
({ viewId }: { viewId: string }) =>
useDatabaseLoading({
viewId,
allowedViewIds: ['view-1', 'child-1', 'child-2'],
// embedded mode should only load meta for the base view
loadMeta,
document: {
id: 'database-1',
view_id: 'view-1',
title: 'Embedded DB',
children: [],
} as any,
}),
{ initialProps: { viewId: 'view-1' } },
);
// base view should trigger one meta load
await waitForNextUpdate();
expect(loadMeta).toHaveBeenCalledTimes(1);
expect(loadMeta).toHaveBeenCalledWith(expect.objectContaining({ viewId: 'view-1' }));
// switch to a child view; embedded mode should not re-load meta
rerender({ viewId: 'child-1' });
// no extra meta loads when switching away from base view
expect(loadMeta).toHaveBeenCalledTimes(1);
expect(result.current.selectedViewId).toBe('child-1');
expect(result.current.visibleViewIds).toEqual(['view-1', 'child-1', 'child-2']);
});
it('should select first child view when viewId not in visible views', async () => {
```
These edits assume the following about the existing test file and hook signature:
1. `renderHook` from `@testing-library/react` and `useDatabaseLoading` are already imported and available.
2. The hook accepts a single configuration object with keys `{ viewId, allowedViewIds, loadMeta, document }`.
3. The hook's `result.current` shape exposes `{ visibleViewIds, selectedViewId, databaseName, notFound }`.
4. `loadMeta` is an injectable async function used by `useDatabaseLoading` to fetch database meta; the tests here inject `jest.fn()` implementations directly.
To fully integrate these tests, you will likely need to:
1. Align the arguments passed to `useDatabaseLoading` with the actual hook signature (rename or add props such as `databaseId`, `client`, or `services` used in the real code).
2. If the existing tests use a custom helper (e.g. `renderUseDatabaseLoading` or a wrapper to provide providers/mocks), update these new tests to use that helper instead of a raw `renderHook` call.
3. Adjust the shape of `document` and the resolved `loadMeta` payloads to match your real meta/document models (e.g. field names like `view_id` vs `viewId`, `children` structure, etc.).
4. Update the expectation on `loadMeta` arguments in the `should skip meta load for non-base views` test if your meta loader is called with different parameters (e.g. `{ databaseId }` instead of `{ viewId }`), or drop that argument assertion if it is too specific.
Once the hook signature and existing helpers are wired up, these tests will exercise:
- Embedded behavior with `allowedViewIds` and meta failures.
- Selection updates when `allowedViewIds` changes.
- `notFound` semantics for embedded vs standalone.
- `visibleViewIds` composition in standalone mode.
- `shouldSkipMetaLoad` behavior for non-base views in embedded databases.
</issue_to_address>
### Comment 4
<location> `src/application/services/js-services/http/__tests__/page.integration.test.ts:62-63` </location>
<code_context>
const rootViewId = outline[0]?.view_id || testWorkspaceId;
const pageName = `Test Page ${Date.now()}`;
- createdPageId = await APIService.addAppPage(testWorkspaceId, rootViewId, {
+ const { view_id } = await APIService.addAppPage(testWorkspaceId, rootViewId, {
layout: 0,
name: pageName,
</code_context>
<issue_to_address>
**suggestion (testing):** Broaden integration tests to assert the new CreatePageResponse shape, including database-specific fields.
These tests now destructure `{ view_id }` from the new `CreatePageResponse`, but they still only assert that `view_id` is a string. To align with the new response shape, please add coverage that checks:
- The response includes the expected keys (`view_id` and `database_id` for database pages).
- For non-database pages, `database_id` is absent or `undefined`, matching the API contract.
This can be done by adding a test that creates a database page (e.g., Grid) and asserts `database_id` is present, or by extending an existing test to assert the presence/absence and value of `database_id` for document vs database pages.
Suggested implementation:
```typescript
const pageName = `Test Page ${Date.now()}`;
const { view_id, database_id } = await APIService.addAppPage(testWorkspaceId, rootViewId, {
layout: 0,
name: pageName,
});
createdPageId = view_id;
expect(createdPageId).toBeDefined();
expect(typeof createdPageId).toBe('string');
// Non-database (document) pages should not be associated with a database
expect(database_id).toBeUndefined();
```
```typescript
const outline = await APIService.getAppOutline(testWorkspaceId);
const rootViewId = outline[0]?.view_id || testWorkspaceId;
const { view_id: pageId, database_id: databaseId } = await APIService.addAppPage(testWorkspaceId, rootViewId, {
```
To fully satisfy the review comment, you should also:
1. Identify the test that creates a database page (e.g., Grid or other database-backed layout). In that test:
- Ensure the `addAppPage` call passes the correct layout/config for a database page.
- After the `addAppPage` call, add assertions like:
```ts
expect(databaseId).toBeDefined();
expect(typeof databaseId).toBe('string');
```
- Optionally, assert that `databaseId` matches any follow-up API calls that operate on that database.
2. If the second `addAppPage` call in the snippet above is *not* the database-page case, locate the appropriate database-page creation test elsewhere in `page.integration.test.ts` and:
- Destructure `{ view_id, database_id }` from its `addAppPage` response.
- Add the expectations described above for database pages.
3. If there are other tests in this file that directly or indirectly depend on the shape of `CreatePageResponse`, consider updating them to destructure `database_id` as needed and assert either presence (for database pages) or absence (for document pages), to keep coverage consistent.
</issue_to_address>
### Comment 5
<location> `src/components/editor/components/blocks/database/hooks/useDatabaseLoading.ts:15` </location>
<code_context>
}
-export const useDatabaseLoading = ({ viewId, loadView, loadViewMeta }: UseDatabaseLoadingProps) => {
+export const useDatabaseLoading = ({ viewId, allowedViewIds, loadView, loadViewMeta }: UseDatabaseLoadingProps) => {
const [notFound, setNotFound] = useState(false);
const [doc, setDoc] = useState<YDoc | null>(null);
</code_context>
<issue_to_address>
**issue (complexity):** Consider pushing the embedded vs standalone behaviors (error handling and initial visible view IDs) further into the loading strategy so the hook becomes a thinner, more linear orchestrator with fewer special cases.
You can keep all the new behavior but reduce the “cross‑cutting” complexity by pushing more of the embedded/standalone logic into the strategy itself and de‑duplicating some branches.
### 1. Let the strategy own “embedded vs standalone” error shaping
Right now, meta error handling is split across:
- `handleError` → `strategy.shouldSetNotFoundOnMetaError()`
- `loadViewMetaWithCallback` → `isEmbedded` special‑cases (including two `if (isEmbedded) return null` paths)
- `useLayoutEffect` → `if (!isEmbedded) setNotFound(true)`
You can centralize this in the strategy by adding a small error‑shaping helper and using it in both `loadViewMetaWithCallback` and the initial meta `useLayoutEffect`.
**Strategy interface (small extension):**
```ts
// loadingStrategies.ts
export interface DatabaseLoadingStrategy {
// ...existing methods
handleMetaError(error: unknown, ctx: { id: string; isInitial: boolean }): {
shouldSetNotFound: boolean;
shouldRethrow: boolean;
metaFallback: View | null | undefined; // e.g. embedded: null, standalone: undefined
};
}
```
**Use it in the hook:**
```ts
const loadViewMetaWithCallback = useCallback(
async (id: string, callback?: (meta: View | null) => void) => {
if (strategy.shouldSkipMetaLoad(id)) {
console.debug('[useDatabaseLoading] Skipping meta load (strategy)', { id, viewId });
return null;
}
try {
const meta = await retryLoadViewMeta(id, id === viewId ? updateVisibleViewIds : callback);
if (meta && id === viewId) {
await updateVisibleViewIds(meta);
}
setNotFound(false);
return meta;
} catch (error) {
const { shouldSetNotFound, shouldRethrow, metaFallback } = strategy.handleMetaError(error, {
id,
isInitial: id === viewId,
});
if (shouldSetNotFound) setNotFound(true);
if (shouldRethrow) throw error;
return metaFallback ?? null;
}
},
[retryLoadViewMeta, strategy, updateVisibleViewIds, viewId]
);
```
```ts
useLayoutEffect(() => {
void loadViewMetaWithCallback(viewId)
.then((meta) => {
if (!viewIdsRef.current.includes(viewId) && viewIdsRef.current.length > 0) {
setSelectedViewId(viewIdsRef.current[0]);
} else {
setSelectedViewId(viewId);
}
if (meta) {
console.debug('[DatabaseBlock] loaded view meta', {
viewId,
children: meta.children?.map((c) => c.view_id),
name: meta.name,
});
}
setNotFound(false);
})
.catch((error) => {
const { shouldSetNotFound } = strategy.handleMetaError(error, {
id: viewId,
isInitial: true,
});
if (shouldSetNotFound) setNotFound(true);
});
}, [loadViewMetaWithCallback, viewId, strategy]);
```
This removes all direct `isEmbedded` branches from the hook and makes the “when do we set `notFound` vs. continue?” logic a single, inspectable place in `loadingStrategies`.
### 2. Move “allowedViewIds on mount” behind the strategy
The combination of:
- `isEmbedded && allowedViewIdsRef.current` in `useLayoutEffect`, and
- the separate `useEffect` syncing `allowedViewIds` → `visibleViewIds`,
makes the lifecycle harder to follow. You can let the strategy express “use block view IDs immediately” instead of hardcoding embedded + `allowedViewIds` checks.
**Extend strategy:**
```ts
export interface DatabaseLoadingStrategy {
// ...
getInitialVisibleViewIds(): string[] | null; // null => let meta decide
}
```
**Hook usage (replacing the `isEmbedded && allowedViewIdsRef.current` block + the extra useEffect):**
```ts
useLayoutEffect(() => {
const initialIds = strategy.getInitialVisibleViewIds();
if (initialIds && initialIds.length > 0) {
setVisibleViewIds(initialIds);
setSelectedViewId((current) =>
initialIds.includes(viewId) ? viewId : initialIds[0] ?? current
);
}
void loadViewMetaWithCallback(viewId)
// ... same as above
}, [loadViewMetaWithCallback, viewId, strategy]);
```
Then the existing `allowedViewIds` sync effect can either be removed or simplified to just update the strategy inputs, depending on how dynamic `allowedViewIds` needs to be.
### 3. Restore a local null guard for `updateVisibleViewIds`
`updateVisibleViewIds` now relies entirely on `strategy.getVisibleViewIds(meta)` handling `null`. Re‑adding a lightweight guard in the hook keeps future strategies from accidentally breaking things:
```ts
const updateVisibleViewIds = useCallback(
async (meta: View | null) => {
if (!meta) return;
const viewIds = strategy.getVisibleViewIds(meta);
const name = strategy.getDatabaseName(meta);
setDatabaseName(name);
setVisibleViewIds(viewIds);
},
[strategy]
);
```
This maintains all current behavior but localizes one important invariant (no meta → no update) back into the hook.
---
These changes keep your strategy abstraction and all embedded/standalone behaviors, but:
- Remove duplicated `isEmbedded` / `allowedViewIds` branching from the hook,
- Centralize error semantics and `notFound` decisions in the strategy,
- Make the hook a thinner orchestrator with clearer, linear control flow.
</issue_to_address>
### Comment 6
<location> `src/components/editor/components/blocks/database/hooks/loadingStrategies.ts:17` </location>
<code_context>
+ * Strategy interface for loading database views
+ * This allows different loading behaviors for embedded vs standalone databases
+ */
+export interface DatabaseLoadingStrategy {
+ /**
+ * Whether meta loading errors should trigger notFound state
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the loading strategy API to a minimal behavior-focused factory and keep straightforward meta-derived transformations directly in the hook instead of hiding them behind strategy methods and duplicate helpers.
You can simplify this without losing any of the behavior you’ve added.
Right now the strategy abstraction is doing three things:
- Holding two booleans (`shouldSetNotFoundOnMetaError`, `shouldSkipMetaLoad`)
- Hiding two simple transformations (`getVisibleViewIds`, `getDatabaseName`)
- Duplicating the “embedded vs standalone” concept with both `createLoadingStrategy` and `isEmbeddedDatabase`
You can keep the configuration flexibility while reducing surface area and indirection by:
1. Returning a minimal shape from `createLoadingStrategy`
2. Moving the simple meta transformations back into the hook (or keeping them as plain helpers)
3. Dropping the separate `isEmbeddedDatabase` export
For example, you can reduce the strategy to just the behavioral flags and keep all data shaping close to where it’s used:
```ts
// types.ts (or same file)
export interface DatabaseLoadingBehavior {
shouldSetNotFoundOnMetaError: boolean;
shouldSkipMetaLoad: (id: string) => boolean;
}
export interface DatabaseLoadingConfig {
viewId: string;
allowedViewIds?: string[];
loadView?: (viewId: string) => Promise<YDoc | null>;
loadViewMeta?: (viewId: string, cb?: (meta: View | null) => void) => Promise<View | null>;
}
export const createLoadingBehavior = (
config: DatabaseLoadingConfig,
): DatabaseLoadingBehavior => {
const { viewId, allowedViewIds } = config;
const isEmbedded = !!allowedViewIds?.length;
if (isEmbedded) {
return {
shouldSetNotFoundOnMetaError: false,
shouldSkipMetaLoad: (id) => id !== viewId,
};
}
return {
shouldSetNotFoundOnMetaError: true,
shouldSkipMetaLoad: () => false,
};
};
```
Then, in `useDatabaseLoading`, you can keep the meta-derived values inline and explicit, instead of hiding them behind `getVisibleViewIds` / `getDatabaseName`:
```ts
const isEmbedded = !!allowedViewIds?.length;
const visibleViewIds = useMemo(() => {
if (isEmbedded) {
return allowedViewIds ?? [];
}
if (!meta) {
return [viewId];
}
const ids = meta.children.map((v) => v.view_id);
ids.unshift(meta.view_id);
return ids;
}, [isEmbedded, allowedViewIds, meta, viewId]);
const databaseName = meta?.name ?? '';
```
This keeps:
- All embedded vs standalone behavior in one place (the hook + a tiny behavior factory)
- No duplication of the “embedded” concept (`isEmbeddedDatabase` becomes unnecessary)
- Simple transformations easy to see and reason about, without extra indirection
The behavior stays identical, but the abstraction layer is thinner and more focused on the parts that are genuinely conditional, not just data shaping.
</issue_to_address>
### Comment 7
<location> `cypress/e2e/embeded/database/embedded-view-isolation.cy.ts:39-58` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 8
<location> `cypress/e2e/embeded/database/embedded-view-isolation.cy.ts:63-70` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 9
<location> `cypress/e2e/embeded/database/embedded-view-isolation.cy.ts:75-81` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 10
<location> `cypress/e2e/embeded/database/embedded-view-isolation.cy.ts:86-93` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 11
<location> `cypress/e2e/embeded/database/embedded-view-isolation.cy.ts:99-106` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 12
<location> `cypress/e2e/embeded/database/embedded-view-isolation.cy.ts:111-129` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 13
<location> `cypress/e2e/embeded/database/embedded-view-isolation.cy.ts:134-141` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 14
<location> `src/components/editor/components/blocks/database/utils/databaseBlockUtils.ts:94-96` </location>
<code_context>
const data = JSON.parse(jsonString) as DatabaseNodeData;
return data;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return JSON.parse(jsonString) as DatabaseNodeData;
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| setReconnectAttempt(attemptNumber); | ||
|
|
||
| // First attempt: random 5-10s delay (thundering herd prevention) | ||
| if (attemptNumber === 1) { |
There was a problem hiding this comment.
issue (bug_risk): Changing the first-attempt check from === 1 to === 0 may skip the intended thundering-herd backoff.
The reconnect handler now treats attemptNumber === 0 as the first attempt, but many websocket libraries use 1 as the first reconnect attempt. If attemptNumber is 1-based, this condition will never run and the first reconnect won’t get the randomized 5–10s delay for herd prevention. Please confirm the library’s attempt numbering and keep this as === 1 (or document the 0-based assumption) if it is 1-based.
josue693
pushed a commit
to josue693/AppFlowy-Web
that referenced
this pull request
Dec 21, 2025
* chore: set databbase id * chore: support display document based embeded database views * chore: filter views * chore: create embedded view * fix: embedded database view * refactor: rename DatabaseContextState * chore: do not show extra when parent is database * chore: linked database base sidebar prefix * chore: add tests * chore: remove unused folder-view api endpoint * chore: fix retry , add tests * chore: select view after creating * chore: clippy * chore: fix test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Support embedding and isolation of database views within documents and clean up related APIs and context types.
New Features:
Bug Fixes:
Enhancements:
Tests:
Chores: