Alerts secondary feature detail fetch 404s: companion table has no view config - #565
Conversation
conservationtimothy
changed the title
| * Same-table reads use `dataTable` (+ optional `viewType`). Cross-table companion | ||
| * reads (e.g. alerts secondary dataset) pass `permissionTable` = the parent view's | ||
| * primary dataset plus `viewType`; `dataTable` must be that view's primary or | ||
| * secondary table. | ||
| * | ||
| * @param {string} dataTable - Warehouse table being read. | ||
| * @param {{ viewType?: ViewType; permissionTable?: string | null }} [options] - Auth scope. | ||
| * @returns {Promise<ViewConfig>} View config for permission checks. | ||
| */ | ||
| export const fetchTableConfigForDataAccess = async ( | ||
| dataTable: string, | ||
| options: { | ||
| viewType?: ViewType; | ||
| permissionTable?: string | null; | ||
| } = {}, | ||
| ): Promise<ViewConfig> => { | ||
| const normalizedDataTable = normalizeTableName(dataTable); | ||
| const permissionTable = options.permissionTable?.trim() | ||
| ? normalizeTableName(options.permissionTable) | ||
| : null; | ||
|
|
||
| if (!permissionTable) { | ||
| return fetchTableConfig(normalizedDataTable, options.viewType); | ||
| } |
There was a problem hiding this comment.
I find "permission table" to be an unclear term for what you are trying to implement across this PR.
What if you called it isSecondaryTable. That would make this block so much clearer, plus what comes next e.g. about fetching primaryTable and secondaryTable.
There was a problem hiding this comment.
I am going to replace permissionTable with primaryDataset, since we need the primary dataset and view type to find and validate the configured secondary dataset.
| // Same-table view reads send view_type; companion reads send permission_table + | ||
| // view_type so the parent alerts/map view authorizes the secondary table. |
There was a problem hiding this comment.
I have read this three times and I don't understand what this is saying at all, and there is semantic overload going on here.
- What is a "same-table view read"?
- What is a "companion read"? (Is this redundant with "secondary table"?)
- Here is a good example of what i meant in my other comment that the term "permission table" really does not clarify anything.
- "so the parent alerts/map view authorizes the secondary table" -- I have no idea what this means. Views don't have "parents" and they don't authorize tables.
Can you make a strong effort to improve the readability of the code AND comments throughout this entire PR?
There was a problem hiding this comment.
Yes, absolutely
| type RecordFetchQuery = { | ||
| view_type?: ViewType; | ||
| primary_dataset?: string; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Remember that we store types in types/index.ts.
2 participants
Goal
Stop the alerts sidebar from 404ing when you click a secondary-dataset point (for example
mapeo_data). Closes #564Screenshots
What I changed and why
/api/<secondary_table>/<id>tried to resolve view config for the secondary table itself, which is never configured as its own viewpermission_table+view_type) alongside secondary record requests/recordscalls follow the same patternHow I convinced myself this is right
The map already had the secondary points; only the follow-up detail request failed, so the data was fine and the permission lookup was wrong. Using the parent alerts view matches how the page is opened. Checking that the secondary table is actually that view’s secondary stops someone from pointing
permission_tableat a public view to read an unrelated table. Same-table fetches still only sendview_type. Tests cover both the own-table and secondary-table query shapes.What I'm not doing here
LLM use disclosure
Cursor Grok 4.5 with me driving