[codex] Enable v0.3 Notion and Postgres onboarding#224
Conversation
📝 WalkthroughWalkthroughThe change adds Notion and Postgres remote onboarding, typed Postgres mutations, shared remote-table materialization, and stricter data-source configuration validation. It also improves clawpatch stale-lock recovery and adds wrapper tests for review-state handling. ChangesRemote data onboarding
Clawpatch review-state handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DataPickerContent
participant connectRemoteSource
participant DataSourceAPI
participant RemoteQueryAPI
participant materializeRemoteTable
User->>DataPickerContent: submit connector credentials
DataPickerContent->>connectRemoteSource: create and probe remote source
connectRemoteSource->>DataSourceAPI: addSource
DataSourceAPI-->>connectRemoteSource: sourceId
connectRemoteSource->>RemoteQueryAPI: list remote resources
RemoteQueryAPI-->>DataPickerContent: discovered resources
User->>DataPickerContent: select resource
DataPickerContent->>RemoteQueryAPI: query selected resource
RemoteQueryAPI-->>DataPickerContent: Arrow query result
DataPickerContent->>materializeRemoteTable: persist remote result
materializeRemoteTable-->>DataPickerContent: materialized table
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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: c79618f5ec
ℹ️ 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".
| "Data source credentials must use typed credential fields, not config", | ||
| ); | ||
| } | ||
| const config: DataSourceConfig = { ...(rawConfig ?? {}) }; |
There was a problem hiding this comment.
When a client sends connector config with a nested or alternate credential value, such as { nested: { connectionString: "postgres://..." } } or { dsn: "postgres://..." }, this spread persists it directly in data_sources.config; rowToDataSource then forwards non-credential config keys back to the renderer. That bypasses the new top-level smuggling guard and breaks the plaintext-never-at-rest/no-secret-in-DTO invariant for any buggy or untrusted caller of addDataSource; whitelist expected non-secret keys like defaultSchema or recursively reject credential-shaped data before copying config.
Useful? React with 👍 / 👎.
| setError( | ||
| cause instanceof Error | ||
| ? cause.message | ||
| : "Failed to import remote table", | ||
| ); |
There was a problem hiding this comment.
When importing a selected Notion/Postgres resource fails here, this sets error, but the only place that renders error is AddConnectionPanel, which is hidden while remoteResourceState is active. In that state, query/materialization failures just re-enable the buttons with no visible message, so users cannot tell why selecting a database/table did nothing; render this error in the Choose data to import branch as well.
Useful? React with 👍 / 👎.
| await addDataFrameEntry(dataFrame, { | ||
| name, | ||
| rowCount: result.rowCount, | ||
| columnCount, | ||
| }); |
There was a problem hiding this comment.
In the new remote import flow, materializeRemoteTable commits a DataFrame entry before the later updateDataTable call links it to the table. If that final update throws after this point, DataPickerContent only removes the newly created DataTable, leaving the DataFrame entry and IndexedDB Arrow blob orphaned; return enough state to delete the frame on failure or make the materialization/linking step atomic.
Useful? React with 👍 / 👎.
| remoteResourceState.resources.map((resource) => ( | ||
| <Button | ||
| key={resource.id} | ||
| label={resource.title} | ||
| variant="outline" | ||
| className="w-full justify-start" | ||
| disabled={materializingResourceId !== null} | ||
| onClick={() => handleRemoteResourceSelect(resource)} |
There was a problem hiding this comment.
When this picker is used by JoinFlowModal, the current base table is excluded only by its existing DataTable id, but this remote-resource list is keyed by the source database/table id and handleRemoteResourceSelect creates a brand-new DataTable id. Reconnecting the same Notion database or Postgres table from the join modal therefore bypasses the self-table exclusion and lets users join the table with a duplicate copy of itself; filter resources whose id matches an excluded table's table value or pass explicit excluded resource ids.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/types/src/data-sources.ts (1)
1-1: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
configis validated by blacklist, not allowlist — same root cause across the type contract and the server handler.CreateDataSourceInput.configis untyped (Record<string, unknown>), and the server only rejects two hardcoded credential key names; any other/future key passes straight through into stored config.
packages/types/src/data-sources.ts#L56-57: narrowconfig's type to the actual known non-credential shape (e.g.{ defaultSchema?: string }, or a per-connector-type discriminated shape) instead ofRecord<string, unknown>.apps/server/src/functions/app-artifacts.ts#L493-501: replace/augment the"apiKey" in rawConfig || "connectionString" in rawConfig"blacklist with an allowlist of recognized non-credential keys (or derive the blocked-key set fromDataSourceConfig's credential fields) so a new credential field can't be smuggled throughconfigwithout this check being updated in lockstep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/types/src/data-sources.ts` at line 1, Restrict CreateDataSourceInput.config in the data-source type contract to the recognized non-credential configuration shape instead of an open Record, and update the validation in the app-artifacts handler around rawConfig to use an allowlist derived from that contract (or equivalent shared credential metadata). Ensure unknown and credential fields are rejected so newly added credential keys cannot pass without the validation being updated.packages/app/src/components/data-sources/DataSourceDisplay.tsx (1)
601-623: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPostgres data sources still hit the unsupported fallback.
DataSourceDisplayonly branches onsourceType === "file"andconnector.id === "notion", so Postgres data sources fall through to “Unsupported data source type.” Add a Postgres/remote-api branch here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/components/data-sources/DataSourceDisplay.tsx` around lines 601 - 623, Update DataSourceDisplay’s connector branching to handle Postgres/remote-api data sources before the unsupported fallback. Route the matching connector to the appropriate remote/Postgres data-source view, while preserving the existing file and Notion branches and fallback behavior for unsupported connectors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/functions/app-artifacts.ts`:
- Around line 493-501: The credential check before constructing DataSourceConfig
must derive forbidden keys from the canonical typed credential fields instead of
hardcoding only apiKey and connectionString. Update the rawConfig validation in
the surrounding data-source creation flow to automatically reject any credential
field, including future additions, while preserving the existing error behavior
and config construction.
In `@packages/app/src/lib/remote-connector-onboarding.ts`:
- Around line 60-63: Update the rollback handling in the catch block around
removeSource so failures from the cleanup attempt are surfaced through the
established logging mechanism before rethrowing the original cause. Preserve the
existing conditional cleanup and ensure the original probe error remains the
thrown error.
In `@packages/app/src/lib/remote-table-materialization.ts`:
- Around line 30-66: Update materializeRemoteTable to track whether
addDataFrameEntry created a new DataFrame, and ensure that if the subsequent
updateDataTable call fails, the newly created DataFrame and its Arrow bytes are
removed using the existing DataFrame cleanup operation. Do not remove an
existing DataFrame when replacing it, and preserve the original failure
propagation.
In `@packages/types/src/data-sources.ts`:
- Around line 56-57: The config field in the data-source type is too permissive
and can diverge from server validation. Replace Record<string, unknown> with the
recognized non-credential configuration shape, at minimum an optional
defaultSchema string, while preserving config’s optional nature and avoiding
credential fields.
In `@scripts/clawpatch.sh`:
- Around line 132-141: Update the changed-file loop in scripts/clawpatch.sh to
check whether each reviewable changed_file still exists before querying the
feature-map JSON files. Skip deleted files, while preserving the existing
unmapped_reviewable_files behavior for files that remain present.
---
Outside diff comments:
In `@packages/app/src/components/data-sources/DataSourceDisplay.tsx`:
- Around line 601-623: Update DataSourceDisplay’s connector branching to handle
Postgres/remote-api data sources before the unsupported fallback. Route the
matching connector to the appropriate remote/Postgres data-source view, while
preserving the existing file and Notion branches and fallback behavior for
unsupported connectors.
In `@packages/types/src/data-sources.ts`:
- Line 1: Restrict CreateDataSourceInput.config in the data-source type contract
to the recognized non-credential configuration shape instead of an open Record,
and update the validation in the app-artifacts handler around rawConfig to use
an allowlist derived from that contract (or equivalent shared credential
metadata). Ensure unknown and credential fields are rejected so newly added
credential keys cannot pass without the validation being updated.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0e409783-b917-4bb0-878f-a4dedccca7f5
📒 Files selected for processing (16)
apps/server/src/functions/app-artifacts.test.tsapps/server/src/functions/app-artifacts.tspackages/app-data/src/index.tspackages/app-data/src/postgres.tspackages/app/src/app/_components/OnboardingView.tsxpackages/app/src/components/data-sources/AddConnectionPanel.tsxpackages/app/src/components/data-sources/DataPickerContent.tsxpackages/app/src/components/data-sources/DataSourceDisplay.tsxpackages/app/src/components/data-sources/renderers/ConnectorCardWithForm.tsxpackages/app/src/lib/connectors/registry.tspackages/app/src/lib/remote-connector-onboarding.test.tspackages/app/src/lib/remote-connector-onboarding.tspackages/app/src/lib/remote-table-materialization.tspackages/types/src/data-sources.tsscripts/clawpatch.shscripts/test-clawpatch-wrapper.sh
| if ( | ||
| rawConfig && | ||
| ("apiKey" in rawConfig || "connectionString" in rawConfig) | ||
| ) { | ||
| throw new Error( | ||
| "Data source credentials must use typed credential fields, not config", | ||
| ); | ||
| } | ||
| const config: DataSourceConfig = { ...(rawConfig ?? {}) }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Blacklist-based credential-smuggling check doesn't scale.
The check hardcodes "apiKey" and "connectionString" as the only forbidden keys. If a future connector introduces a new credential field (e.g. an OAuth token), it must be added here manually or it can be smuggled through config and persisted in plaintext. Prefer deriving the blocked-key set from the canonical credential fields (or switch to an allowlist of known non-credential keys per connector type) so this stays in sync automatically.
🤖 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 `@apps/server/src/functions/app-artifacts.ts` around lines 493 - 501, The
credential check before constructing DataSourceConfig must derive forbidden keys
from the canonical typed credential fields instead of hardcoding only apiKey and
connectionString. Update the rawConfig validation in the surrounding data-source
creation flow to automatically reject any credential field, including future
additions, while preserving the existing error behavior and config construction.
| } catch (cause) { | ||
| if (sourceId) await removeSource(sourceId).catch(() => {}); | ||
| throw cause; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Rollback failures are silently swallowed.
If removeSource fails after a probe error, the failure is discarded with no logging, so a data source can be left orphaned with no trace to diagnose it later.
🩹 Optional: surface rollback failures
} catch (cause) {
- if (sourceId) await removeSource(sourceId).catch(() => {});
+ if (sourceId) {
+ await removeSource(sourceId).catch((rollbackErr) => {
+ console.error("connectRemoteSource: rollback failed", sourceId, rollbackErr);
+ });
+ }
throw cause;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (cause) { | |
| if (sourceId) await removeSource(sourceId).catch(() => {}); | |
| throw cause; | |
| } | |
| } catch (cause) { | |
| if (sourceId) { | |
| await removeSource(sourceId).catch((rollbackErr) => { | |
| console.error("connectRemoteSource: rollback failed", sourceId, rollbackErr); | |
| }); | |
| } | |
| throw cause; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/app/src/lib/remote-connector-onboarding.ts` around lines 60 - 63,
Update the rollback handling in the catch block around removeSource so failures
from the cleanup attempt are surfaced through the established logging mechanism
before rethrowing the original cause. Preserve the existing conditional cleanup
and ensure the original probe error remains the thrown error.
| /** Persist a server-returned Arrow result and link it to its DataTable. */ | ||
| export async function materializeRemoteTable( | ||
| table: { id: string }, | ||
| result: RemoteQueryResult, | ||
| name: string, | ||
| ): Promise<MaterializedRemoteTable> { | ||
| const columnCount = result.fieldIds.length; | ||
| const dataFrame = await DataFrame.create( | ||
| decodeBase64ToBytes(result.arrowBuffer), | ||
| result.fieldIds as UUID[], | ||
| ); | ||
| const existing = await getDataTable(table.id as UUID); | ||
| let dataFrameId: UUID; | ||
|
|
||
| if (existing?.dataFrameId) { | ||
| await replaceDataFrame(existing.dataFrameId, dataFrame, { | ||
| rowCount: result.rowCount, | ||
| columnCount, | ||
| }); | ||
| dataFrameId = existing.dataFrameId; | ||
| } else { | ||
| await addDataFrameEntry(dataFrame, { | ||
| name, | ||
| rowCount: result.rowCount, | ||
| columnCount, | ||
| }); | ||
| dataFrameId = dataFrame.id as UUID; | ||
| } | ||
|
|
||
| await updateDataTable(table.id as UUID, { | ||
| fields: result.fields, | ||
| dataFrameId, | ||
| lastFetchedAt: Date.now(), | ||
| }); | ||
|
|
||
| return { dataFrameId, rowCount: result.rowCount, columnCount }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether removing a DataTable cascades deletion of its linked/orphaned DataFrame entries.
rg -nP -C6 '\bremoveDataTable\b|\bdeleteDataFrameEntry\b' packages/app-data/srcRepository: youhaowei/DashFrame
Length of output: 1120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relevant files ==\n'
git ls-files 'packages/app/src/lib/remote-table-materialization.ts' 'packages/app/src/lib/*' 'packages/app-data/src/*' | sed -n '1,200p'
printf '\n== remote-table-materialization.ts ==\n'
cat -n packages/app/src/lib/remote-table-materialization.ts
printf '\n== data-tables.ts ==\n'
cat -n packages/app-data/src/data-tables.ts
printf '\n== search for DataFrame cleanup / removeDataTable usage ==\n'
rg -n -C4 '\b(removeDataTable|deleteDataFrameEntry|deleteDataFrame|removeDataFrame|replaceDataFrame|addDataFrameEntry|updateDataTable)\b' packages/app-data/src packages/app/src/libRepository: youhaowei/DashFrame
Length of output: 31277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== remote-table-materialization.ts =='
sed -n '1,220p' packages/app/src/lib/remote-table-materialization.ts
echo
echo '== data-tables.ts =='
sed -n '1,220p' packages/app-data/src/data-tables.ts
echo
echo '== package app-data files mentioning DataFrame/DataTable cleanup =='
rg -n -C3 'DataFrame|DataTable|deleteDataFrameEntry|removeDataTable|refreshDataTable|addDataFrameEntry|replaceDataFrame|updateDataTable' packages/app-data/srcRepository: youhaowei/DashFrame
Length of output: 32461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== find DataPickerContent.handleRemoteResourceSelect =='
rg -n -C6 'handleRemoteResourceSelect|materializeRemoteTable|removeDataTable|rollback' packages/app/src packages/app-data/src
echo
echo '== list files with DataPickerContent =='
fd -i 'DataPickerContent' packages/app/src packages/app-data/src
echo
echo '== list files with handleRemoteResourceSelect references =='
rg -n 'handleRemoteResourceSelect' packages/app/src packages/app-data/srcRepository: youhaowei/DashFrame
Length of output: 13486
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== locate removeDataTable implementation =='
rg -n -C6 'removeDataTable' packages -g '!**/*.test.*'
echo
echo '== locate DataTable/DataFrame schema fields =='
rg -n -C4 'dataFrameId|removeDataFrameEntry|removeDataTableEntry|putDataFrameEntry|updateDataTable' packages/app-data packages/app-core packagesRepository: youhaowei/DashFrame
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '250,305p' packages/app/src/components/data-sources/DataPickerContent.tsxRepository: youhaowei/DashFrame
Length of output: 2011
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== removeDataTable / removeDataFrame command definitions =='
rg -n -C4 'removeDataTableEntry|removeDataFrameEntry|removeDataTable\b|removeDataFrame\b' packages --glob '!**/*.test.*'
echo
echo '== data-table schema and commands around dataFrameId =='
rg -n -C4 'dataFrameId|refreshDataTable|updateDataTable|createDataTable|addDataTable' packages --glob '!**/*.test.*'Repository: youhaowei/DashFrame
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== removeDataTableEntry / removeDataFrameEntry references (limited) =='
rg -n -C3 --max-count 20 'removeDataTableEntry|removeDataFrameEntry|removeDataTable\b|removeDataFrame\b' .
echo
echo '== data-table / data-frame command definitions in app-data src =='
rg -n -C3 --max-count 40 'addDataTable|createDataTable|updateDataTable|refreshDataTable|removeDataTable|putDataFrameEntry|updateDataFrameEntry|removeDataFrameEntry' packages/app-data/srcRepository: youhaowei/DashFrame
Length of output: 19036
Clean up the DataFrame when remote materialization fails. If addDataFrameEntry succeeds and updateDataTable throws, the rollback only removes the DataTable row; removeDataTable does not delete the newly created DataFrame or its Arrow bytes, so this can leave an orphaned frame behind.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/app/src/lib/remote-table-materialization.ts` around lines 30 - 66,
Update materializeRemoteTable to track whether addDataFrameEntry created a new
DataFrame, and ensure that if the subsequent updateDataTable call fails, the
newly created DataFrame and its Arrow bytes are removed using the existing
DataFrame cleanup operation. Do not remove an existing DataFrame when replacing
it, and preserve the original failure propagation.
| /** Connector-specific, non-credential settings. */ | ||
| config?: Record<string, unknown>; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Loosely-typed config weakens the connector-config contract.
config?: Record<string, unknown> accepts any shape, while the server-side DataSourceConfig only recognizes defaultSchema as a non-credential field. Nothing here ties the two together, so a typo'd or future field can silently pass through server validation (which only blacklists apiKey/connectionString). Consider a narrower type (e.g. { defaultSchema?: string } or a per-connector discriminated shape) to keep the client/server contract in sync.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/types/src/data-sources.ts` around lines 56 - 57, The config field in
the data-source type is too permissive and can diverge from server validation.
Replace Record<string, unknown> with the recognized non-credential configuration
shape, at minimum an optional defaultSchema string, while preserving config’s
optional nature and avoiding credential fields.
| unmapped_reviewable_files="" | ||
| while IFS= read -r changed_file; do | ||
| case "$changed_file" in | ||
| *.ts|*.tsx|*.js|*.jsx|*.mjs|*.cjs|*.go|*.py|*.rb|*.ex|*.exs|*.rs|*.cs|*.cpp|*.cc|*.c|*.h|*.hpp|*.swift|*.java|*.kt|*.kts|*.php|*.vue|*.svelte) | ||
| if ! grep -Fq -- "\"path\": \"$changed_file\"" "$state_dir"/features/*.json 2>/dev/null; then | ||
| unmapped_reviewable_files+="${unmapped_reviewable_files:+$'\n'}$changed_file" | ||
| fi | ||
| ;; | ||
| esac | ||
| done <<<"$changed_files" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude deleted files from the unmapped files check.
If a reviewable file is deleted, git diff still includes it in changed_files. Since a deleted file will no longer be mapped in the features JSON after updating the map, this loop will incorrectly flag it as an unmapped file and cause a false failure. Check for file existence before querying the feature map to safely ignore deletions.
🐛 Proposed fix
*.ts|*.tsx|*.js|*.jsx|*.mjs|*.cjs|*.go|*.py|*.rb|*.ex|*.exs|*.rs|*.cs|*.cpp|*.cc|*.c|*.h|*.hpp|*.swift|*.java|*.kt|*.kts|*.php|*.vue|*.svelte)
- if ! grep -Fq -- "\"path\": \"$changed_file\"" "$state_dir"/features/*.json 2>/dev/null; then
+ if [[ -e "$changed_file" ]] && ! grep -Fq -- "\"path\": \"$changed_file\"" "$state_dir"/features/*.json 2>/dev/null; then
unmapped_reviewable_files+="${unmapped_reviewable_files:+$'\n'}$changed_file"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unmapped_reviewable_files="" | |
| while IFS= read -r changed_file; do | |
| case "$changed_file" in | |
| *.ts|*.tsx|*.js|*.jsx|*.mjs|*.cjs|*.go|*.py|*.rb|*.ex|*.exs|*.rs|*.cs|*.cpp|*.cc|*.c|*.h|*.hpp|*.swift|*.java|*.kt|*.kts|*.php|*.vue|*.svelte) | |
| if ! grep -Fq -- "\"path\": \"$changed_file\"" "$state_dir"/features/*.json 2>/dev/null; then | |
| unmapped_reviewable_files+="${unmapped_reviewable_files:+$'\n'}$changed_file" | |
| fi | |
| ;; | |
| esac | |
| done <<<"$changed_files" | |
| unmapped_reviewable_files="" | |
| while IFS= read -r changed_file; do | |
| case "$changed_file" in | |
| *.ts|*.tsx|*.js|*.jsx|*.mjs|*.cjs|*.go|*.py|*.rb|*.ex|*.exs|*.rs|*.cs|*.cpp|*.cc|*.c|*.h|*.hpp|*.swift|*.java|*.kt|*.kts|*.php|*.vue|*.svelte) | |
| if [[ -e "$changed_file" ]] && ! grep -Fq -- "\"path\": \"$changed_file\"" "$state_dir"/features/*.json 2>/dev/null; then | |
| unmapped_reviewable_files+="${unmapped_reviewable_files:+$'\n'}$changed_file" | |
| fi | |
| ;; | |
| esac | |
| done <<<"$changed_files" |
🤖 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 `@scripts/clawpatch.sh` around lines 132 - 141, Update the changed-file loop in
scripts/clawpatch.sh to check whether each reviewable changed_file still exists
before querying the feature-map JSON files. Skip deleted files, while preserving
the existing unmapped_reviewable_files behavior for files that remain present.
| setError( | ||
| cause instanceof Error | ||
| ? cause.message | ||
| : "Failed to import remote table", | ||
| ); |
There was a problem hiding this comment.
This still copies arbitrary runtime Error.message text into the picker error state. Failures from Notion or Postgres mutations, vault-backed connector calls, Arrow decoding, or DataFrame persistence can include connector details or internal runtime text, and the picker renders that string directly. A failed remote import should show a fixed recovery message instead of exposing the raw error.
Context Used: NO RAW RUNTIME ERRORS IN USER-FACING UI (invariant... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/app/src/components/data-sources/DataPickerContent.tsx
Line: 297-301
Comment:
**Raw Errors Reach UI**
This still copies arbitrary runtime `Error.message` text into the picker error state. Failures from Notion or Postgres mutations, vault-backed connector calls, Arrow decoding, or DataFrame persistence can include connector details or internal runtime text, and the picker renders that string directly. A failed remote import should show a fixed recovery message instead of exposing the raw error.
**Context Used:** NO RAW RUNTIME ERRORS IN USER-FACING UI (invariant... ([source](greptile.json))
How can I resolve this? If you propose a fix, please make it concise.
| const dataFrame = await DataFrame.create( | ||
| decodeBase64ToBytes(result.arrowBuffer), | ||
| result.fieldIds as UUID[], | ||
| ); |
There was a problem hiding this comment.
This still creates the browser DataFrame from the full remote Arrow payload before any field-sensitivity decision is applied. DataFrame.create(...) writes the decoded bytes to persistent browser storage, and result.fields is only stored later through updateDataTable(...). When a newly imported Notion or Postgres table contains absent, unclassified, or sensitive fields, raw row values can already be durable at rest before the cleared-only cache policy can reject them.
Context Used: NO OFF-TOKEN COLOR (invariant). No raw hex/rgb/okl... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/app/src/lib/remote-table-materialization.ts
Line: 37-40
Comment:
**Rows Persist Before Gate**
This still creates the browser `DataFrame` from the full remote Arrow payload before any field-sensitivity decision is applied. `DataFrame.create(...)` writes the decoded bytes to persistent browser storage, and `result.fields` is only stored later through `updateDataTable(...)`. When a newly imported Notion or Postgres table contains absent, `unclassified`, or `sensitive` fields, raw row values can already be durable at rest before the cleared-only cache policy can reject them.
**Context Used:** NO OFF-TOKEN COLOR (invariant). No raw hex/rgb/okl... ([source](greptile.json))
How can I resolve this? If you propose a fix, please make it concise.
| } | ||
|
|
||
| if (existing?.dataFrameId && existing.dataFrameId !== dataFrame.id) { | ||
| await removeDataFrame(existing.dataFrameId); |
There was a problem hiding this comment.
When a table already has a frame, this cleanup runs after the new frame entry is created and the table has been updated to point at it. If removing the old frame fails, the error escapes to callers even though the replacement is already committed. The data picker then treats the import as failed and removes the new dataFrameId, leaving the table pointing at a frame that no longer exists.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/app/src/lib/remote-table-materialization.ts
Line: 91
Comment:
**Cleanup Deletes Replacement**
When a table already has a frame, this cleanup runs after the new frame entry is created and the table has been updated to point at it. If removing the old frame fails, the error escapes to callers even though the replacement is already committed. The data picker then treats the import as failed and removes the new `dataFrameId`, leaving the table pointing at a frame that no longer exists.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Verification
bun check— 84/84 tasks passedbun run test— 44/44 tasks passedbun run typecheck— 49/49 tasks passedscripts/test-clawpatch-wrapper.shbun run clawpatch:review— exits 0; diff mapped and no eligible features remainTracker
Greptile Summary
This PR enables remote connector onboarding and refresh paths. The main changes are:
Confidence Score: 4/5
This is close, but the replacement-frame cleanup should be fixed before merging.
packages/app/src/lib/remote-table-materialization.ts
Important Files Changed
Prompt To Fix All With AI
Reviews (9): Last reviewed commit: "fix: close remote onboarding review gaps" | Re-trigger Greptile
Context used (4)