Repository navigation
Conversation
albert20260301
left a comment
There was a problem hiding this comment.
The required browser suite is failing in browser-tests/bridge-source.spec.ts on both desktop and phone. After clicking Data query, the iframe still shows the prior virtual-state result ({"period":"last-7","version":1}), not the query result the test expects.
Please update the bridge fixture and coverage so this path either exercises the new registered data:query contract (including the operation ID and values) or explicitly preserves and verifies the legacy data:sql path. The current failure leaves the feature's route-selection and backward-compatibility behavior unverified.
| return null; | ||
| } | ||
| const router = createMessageRouter( | ||
| const legacyRouter = createMessageRouter( |
There was a problem hiding this comment.
We can remove all things legacy
| missingOption?: ComboboxOption; | ||
| disabled?: boolean; | ||
| placeholder?: string; | ||
| searchable?: boolean; |
There was a problem hiding this comment.
This isn't semantically correct. A combobox is always searchable. We should use another component when we don't need search.
| valid() { | ||
| return true; // Query values are validated by the backend. | ||
| }, |
| from: range.start ? new Date(`${range.start}T00:00:00.000Z`) : null, | ||
| to: range.end ? new Date(`${range.end}T23:59:59.999Z`) : null, |
There was a problem hiding this comment.
Perhaps could extract date utils for agents to reuse across the app: startOfDay(), endOfDay().
|
|
||
| /** Display URL or controlled values without making malformed values crash the UI. */ | ||
| export function formatVariableDate( | ||
| value: AbsoluteOrRelativeDateTime | null | undefined |
There was a problem hiding this comment.
Do we still need to carry these GraphQL dependent typings here? Ideally we narrow down possible types.
| onChange, | ||
| nullable = false, | ||
| options, | ||
| }: ValueProps<string | number> & { type: 'STRING' | 'INTEGER' | 'FLOAT' }) { |
There was a problem hiding this comment.
We should have a single type dictionary and pick from there so that types flow down.
| type === 'STRING' | ||
| ? 'Enter a value' | ||
| : `Enter a valid ${type === 'INTEGER' ? 'integer' : 'number'}` |
There was a problem hiding this comment.
Could use a map instead of fragile conditionals that might break with additional data types. We can type check them.
| function option(value: boolean | null) { | ||
| return choice(value, value === null ? 'Null' : value ? 'True' : 'False'); | ||
| } |
| option(value) | ||
| )} | ||
| onChange={next => | ||
| onChange(JSON.parse(next) as VariableValue<'INTERVAL'> | null) |
There was a problem hiding this comment.
We could provide a safe parsing function that casts?
|
@robinvrd the current head now conflicts with Please rebase and resolve the conflict before the next review. The required source/package and browser checks need to rerun on that resolved head, and François's active design-review threads still need to be addressed or resolved. |
|
So following https://github.com/altertable-ai/backend/pull/4571 I think we can remove the Then we will need to update the frontend and finally the skills I believe |
|
We can also update the bun proxy logic |
|
We can also update the bun proxy logic |
|
@leonkenneth @francoischalifour #33 is still conflicting and has active review feedback. Can you confirm whether the intended scope is now to remove hosted |
Registered queries now use DuckDB prepared-statement parameters ($name), so apps only register queries and pass a name-to-value hash. Removes the variable definitions, queryVariable(), and the variable selectors; the date pickers keep their own browser coverage. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Resolve conflicts against main's styling, view, and Vitest refactors: - Take main's DateRangePicker, Combobox, and react exports; drop the branch's DatePicker, CalendarContent, and open-range picker API, whose only consumers were the removed variable selectors. - Port registered-query coverage into tests/public-api and route the hosted starter through registeredQueryRoute in the test host. - Keep main's starter, re-applying only the registered query call. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…variables # Conflicts: # dev/fixtures/bundle-host.tsx # examples/starter-data-app/index.tsx
| Follow the [hosted build skill](https://github.com/altertable-ai/skills/blob/main/skills/build-data-app/SKILL.md) | ||
| for the create/update tool workflow. |
There was a problem hiding this comment.
This repo should be unaware of the skills.
| - [queries.json](../examples/starter-data-app/queries.json): maps query IDs to SQL | ||
| statements with [DuckDB parameters](contract.md#execute-named-queries) such as | ||
| `$groupName`. Keep this map out of the browser bundle. |
There was a problem hiding this comment.
Isn't this a local data app concept?
| 2. Move SQL from local server operations into `queries.json`, replace interpolated | ||
| inputs with `$name` parameters, and replace statement calls with | ||
| `query(id, values)`. |
There was a problem hiding this comment.
Ideally we provide a function for this, so that it's fast, determinist, and doesn't spend tokens.
Summary
Hosted data apps execute registered queries by ID and variable values through the existing bridge. SQL templates stay on the host.
name,type,nullable,default, and optionaloptions, matching the frontend's supported types.data:querypayload{ operation, variables, limit }using the existing bridge.queries.json, andvariables.json.Variable and SQL parsing, registration validation, defaults, and query execution belong to the backend. This package supplies TypeScript types, selectors, and transport; the hosted preview uses mock backend results.
Local CLI apps keep their existing HTTP operations and server-owned SQL flow.
Hosted flow
query(queryId, values); the bridge forwards the ID and values to the frontend.runDataAppQuerywith the trusted app slug, query ID, and values. The backend resolves the saved SQL, validates and binds values, authorizes access, and executes.The frontend and create/update integration are coordinated with frontend #2766 and skills #87.
Validation
bun run checkwith Bun 1.4.2 passed, including 188 tests and local starter verification.