feat(app): [YW-273] add-remote-source renderer flow — DataSourceDisplay preview/query UI#148
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 33 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: 756cd4b54c
ℹ️ 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Resolved all open review threads in commit 3317577 (head 3317577):
Verification: an added Notion source now survives a reload with data AND schema intact (frame in IndexedDB + tracked entry, fields + dataFrameId on the table). Local gate: |
…w/query UI
Wire the Notion data-plane in DataSourceDisplay: replace the NOTION_ENABLED=false
gate and pendingSchema/pendingQueryResult stubs with real server calls via
useNotionMutations() from @dashframe/core. The sync path calls
queryNotionDatabase server-side (credential resolves via the bound resolver,
never enters the renderer) and returns { arrowBuffer, fieldIds, fields } — a
serializable result the UI uses to show a column-schema preview. No credential
or resolver is in renderer scope (verified by type: no secret-vault import,
no apiKey, no ref in DataSourceDisplay.tsx).
Remove property-selection/row-limit UI (MultiSelectField, InputField, schema
useEffect, isFetchingSchema/databaseSchema/selectedPropertyIds/rowLimit state)
and the deferred stubs that threw at runtime. The sync button now calls
queryDatabase directly; preview shows column definitions (rows stay server-side
in the Arrow buffer until a visualization materializes them).
Un-gate NOTION_ENABLED=true: the deferred banner is removed from the Notion
data-source display path now that the server seam is complete.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The serializable query result (arrowBuffer + fieldIds + fields) carries no materialized row count — rows stay in the Arrow buffer server-side until a visualization materializes them. The previous code set rowCount to fieldIds.length (a column count), producing misleading "12 rows" labels. Fix: - PreviewData.rowCount is now optional (undefined = column-schema-only view) - getPreviewDescription shows "N columns" when rowCount is undefined - getTableStatsDescription shows "N columns" (no rows × prefix) when rowCount is undefined; "No data synced yet" only when columnCount is absent - Both sync/refresh handlers omit rowCount rather than setting it to the field count Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
notionPreviewData was shared across all selected tables — switching from Table A (synced) to Table B would still show Table A's column schema. Fix: key PreviewData with tableId and derive currentPreviewData by comparing notionPreviewData.tableId to selectedDataTable.id; returns null when they differ. The preview card, header stats, and collapse toggle all use currentPreviewData so the stale view is suppressed immediately on table switch rather than waiting for the next sync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ionDataSourceView + useNotionSync hook sonarjs/cognitive-complexity was 16 (limit 15) on DataSourceDisplay after adding the Notion preview flow. Fix: move Notion-specific state, query logic and JSX into NotionDataSourceView + useNotionSync so each function stays within budget. Also extract formatRelativeTime to module scope (was a useCallback inside the component). DataSourceDisplay becomes a thin dispatcher (local / notion / unsupported), complexity drops to ~6. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…; drop dead flag
Closes the two P1 review findings that left an added Notion source broken on
reload:
- Persist the frame BEFORE reporting success. New materializeNotionTable()
decodes the server's base64 Arrow buffer, persists it to IndexedDB via
DataFrame.create, registers (or replaces) the DataFrame entry, and links it
to the DataTable. Mirrors the local-CSV ingest path. Replaces the old
refresh() call that only stamped lastFetchedAt and never persisted the frame.
- Save returned fields onto the DataTable via updateDataTable({ fields,
dataFrameId, lastFetchedAt }), so the table has a real schema after add.
Both now survive a reload with data and schema intact.
Supporting changes:
- ConnectorQueryResult / server + app-data NotionQueryResult carry rowCount
(single ConnectorQueryResult implementor: NotionConnector — updated).
- queryNotionDatabase gains an optional limit arg, forwarded as pagination.
Server clamps to positive integers — limit:0 must not become an unbounded
scan via the connector's `0 || Infinity` page loop.
- Sync materializes the FULL database (no row cap) so large sources persist
intact; the limit primitive stays for a future preview-only fetch.
Cleanup (no-backward-compat): NOTION_ENABLED is now always true and its banner
branch is dead in every caller — removed the flag, the NotionDeferredBanner
component, and the stale ConnectorSetup comment.
Local gate: turbo check 75/75. Added route tests for limit forwarding,
unbounded default, and the limit:0 clamp; connector test asserts rowCount.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eryResult contract ConnectorQueryResult now requires rowCount (added for the Notion frame-persist flow). connector-rest is the second implementor of query() — return limitedRows.length so the shared contract holds. Surfaced by CI after rebasing onto main, which introduced connector-rest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
youhaowei
force-pushed
the
charixandra/yw-273-add-remote-source-renderer-flow
branch
from
3317577 to
8f60958
Compare
1 participant
Summary
NOTION_ENABLED=false/ stubpendingSchema()/pendingQueryResult()with real server calls viauseNotionMutations()— the full add-remote-source journey is now live end-to-endNOTION_ENABLED=true: the deferred banner is gone from the Notion data-source display path; the server seam (listNotionDatabases+queryNotionDatabase) was already complete and tested on mainqueryNotionDatabasereturns{ arrowBuffer, fieldIds, fields }— the renderer builds a column list fromfieldsand shows it in the preview card. Rows stay in the Arrow buffer server-side until a visualization materializes them (by design)MultiSelectField+ schemauseEffect+isFetchingSchema/databaseSchema/selectedPropertyIds/rowLimitstate are gone — no separate schema endpoint exists on the server, and the query result already carries the full field list. Property filtering is a follow-on (the server query can acceptselectedPropertyIdsin a future ticket)Privacy boundary — VERIFIED
DataSourceDisplay.tsxhas:@wystack/secret-vaultimportSecretRef,SecretVault,apiKey, or plaintext credential in scopedataSourceId(a public UUID) — the server reads theSecretReffrom the DB row, mints the bound resolver, and calls the connector. The renderer only receives{ arrowBuffer, fieldIds, fields }.Test plan
bun check— 75/75 tasks clean (verified locally)apps/servervitest — 8 files, 236 tests passing (includesnotion-routes.test.tsand vault control-plane tests)packages/connector-notionvitest — 2 files, 81 tests passing (includes capability-attenuation / cross-resolver access test)DataSourceDisplay.tsxforsecret\|apiKey\|SecretRef\|SecretVault\|plaintext→ no hits (onlyconfig.hasApiKeypresence check)🤖 Generated with Claude Code
Greptile Summary
This PR wires up the full Notion renderer flow end-to-end:
DataSourceDisplaynow calls real server mutations (queryNotionDatabase/listNotionDatabases) viauseNotionMutations(), replacing the deferred stub code that was gated behindNOTION_ENABLED=false. TheNotionDeferredBannerfeature-flag file is deleted andNotionDataSourceView+useNotionSyncare extracted to keep the main component within its cognitive-complexity budget.rowCountadded toConnectorQueryResultand threaded throughconnector-notion,connector-rest,app-artifacts, and theapp-dataclient type so the renderer can display accurate row counts and register DataFrame metadata.limitparameter added toqueryNotionDatabase(server + hook) with a defensive guard that rejectslimit ≤ 0; three new tests cover the wiring, unbounded fetch, and zero-limit rejection.materializeNotionTablepersists the Arrow buffer to IndexedDB and updates the DataTable record (fields + dataFrameId + lastFetchedAt) before showing the success toast, so synced data survives a reload;handleSyncDataandhandleRefreshDataTableare unified intorunNotionQueryeliminating ~30 lines of duplication; stale preview on table switch is suppressed by keyingPreviewDataontableId.Confidence Score: 5/5
Safe to merge — the renderer-to-server Notion query path is complete and correct, all previous review findings are addressed, and the new persistence logic is well-guarded.
The rowCount fix, stale-preview suppression, handler deduplication, and missing column-guard are all resolved from prior rounds. The materializeNotionTable function correctly handles both first-sync and re-sync cases (replace-in-place vs. add). The optional limit parameter has a correct non-positive guard backed by three targeted tests. No off-token colors, no ticket IDs in source, and the privacy invariant (no credentials in renderer scope) is maintained throughout.
No files require special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant UI as NotionDataSourceView participant Hook as useNotionSync participant AppData as useNotionMutations participant Server as queryNotionDatabase (server) participant Connector as NotionConnector participant IDB as IndexedDB (DataFrame) UI->>Hook: handleSyncData() Hook->>Hook: runNotionQuery() Hook->>AppData: queryDatabase(dataSourceId, databaseId, tableId) AppData->>Server: "mutateAsync({ dataSourceId, databaseId, tableId })" Server->>Connector: connector.query(databaseId, tableId, pagination?) Connector-->>Server: "{ arrowBuffer, fieldIds, fields, rowCount }" Server-->>AppData: NotionQueryResult AppData-->>Hook: result Hook->>Hook: filter fields (strip _-prefixed) Hook->>IDB: materializeNotionTable(table, result, name) IDB->>IDB: DataFrame.create(bytes, fieldIds) IDB->>IDB: replaceDataFrame OR addDataFrameEntry IDB->>IDB: updateDataTable(fields, dataFrameId, lastFetchedAt) IDB-->>Hook: "{ dataFrameId, rowCount, columnCount }" Hook->>Hook: "setNotionPreviewData({ tableId, columns, rowCount })" Hook-->>UI: "isRefreshing=false, notionPreviewData updated" UI->>UI: "currentPreviewData = previewData if tableId matches"%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant UI as NotionDataSourceView participant Hook as useNotionSync participant AppData as useNotionMutations participant Server as queryNotionDatabase (server) participant Connector as NotionConnector participant IDB as IndexedDB (DataFrame) UI->>Hook: handleSyncData() Hook->>Hook: runNotionQuery() Hook->>AppData: queryDatabase(dataSourceId, databaseId, tableId) AppData->>Server: "mutateAsync({ dataSourceId, databaseId, tableId })" Server->>Connector: connector.query(databaseId, tableId, pagination?) Connector-->>Server: "{ arrowBuffer, fieldIds, fields, rowCount }" Server-->>AppData: NotionQueryResult AppData-->>Hook: result Hook->>Hook: filter fields (strip _-prefixed) Hook->>IDB: materializeNotionTable(table, result, name) IDB->>IDB: DataFrame.create(bytes, fieldIds) IDB->>IDB: replaceDataFrame OR addDataFrameEntry IDB->>IDB: updateDataTable(fields, dataFrameId, lastFetchedAt) IDB-->>Hook: "{ dataFrameId, rowCount, columnCount }" Hook->>Hook: "setNotionPreviewData({ tableId, columns, rowCount })" Hook-->>UI: "isRefreshing=false, notionPreviewData updated" UI->>UI: "currentPreviewData = previewData if tableId matches"Comments Outside Diff (2)
packages/app/src/components/data-sources/DataSourceDisplay.tsx, line 352-447 (link)handleSyncDataandhandleRefreshDataTableare near-identicalBoth handlers call
notionMutations.queryDatabasewith the same arguments, apply the samefieldsfilter/map, write the samenotionPreviewDatashape, and calltableMutations.refresh. The only difference is the success toast message. Extracting the shared logic into a privaterunNotionQueryhelper would eliminate ~30 lines of duplication and make future changes (e.g. surfacing row count when the API returns it) a one-line edit.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
packages/app/src/components/data-sources/DataSourceDisplay.tsx, line 444-461 (link)handleRefreshDataTableis missing the!columns.lengthguard thathandleSyncDatahas (line 395). If a refresh returns a Notion database where every field name begins with_(e.g. after a schema change),columnswill be empty,setNotionPreviewDatawill overwrite the previously valid preview with{ rows: [], columns: [] },tableMutations.refreshwill update the timestamp, and the user sees "Data refreshed successfully" — but the preview card goes blank with "0 columns".handleSyncDataavoids this by bailing out early and leaving the existing preview intact.Prompt To Fix With AI
Reviews (5): Last reviewed commit: "fix(connector-rest): return rowCount fro..." | Re-trigger Greptile