alex-poor commented on PR #40679: URL: https://github.com/apache/superset/pull/40679#issuecomment-5655715102
Worked through the rest of the Bito suggestions from run #79f36b — all addressed in 03df270 and 843f5e2, except one I'm declining with reasons below. **Done** - **Stale translation discarded** (`views/CRUD/hooks.ts`) — you were right and my override was wrong. `PropertiesModal` re-fetches the chart after the PUT and the chart GET dumps `ChartGetResponseSchema`, which declares `localized_name`, so `edits` already carried a translation resolved for the new name. Clearing it showed the canonical name — the opposite of the intent. Removed. - **Batch query param limit** — chunked at 500. Worth noting the failure mode was as bad as you describe: the raise was swallowed by the handler above, so a large dashboard would have silently lost every translation on the page rather than erroring. - **Dead `or ""` fallback** — now `or default_text`, matching what `translate()` guarantees. - **Non-atomic per-row commit** in the seed example — the helper now stages rows and the caller owns one transaction, rolling back on failure. - **Misleading "present only when enabled" comments** — correct, and it applied to five sites, not two (`types/Chart.ts`, `types/Dashboard.ts`, `dashboard/types.ts`, `views/CRUD/types.ts`, `pages/DashboardList/index.tsx`). All reworded to describe the mirroring. - **Missing localized-title tests** — added for both the dashboard header (shows the translation, falls back once a rename outdates it, edits target the canonical title) and the dashboard list (renders and falls back). - **Docstring/behavior mismatch** on the request memo — corrected; background jobs short-circuit on a null locale rather than resolving directly. **Declining: the `pre_get_list` prefetch for list endpoints** The N+1 is real, but that hook cannot fix it. In FAB's `get_list_headless` the order is: ```python response[API_RESULT_RES_KEY] = list_model_schema.dump(lst, many=True) response["ids"] = pks response["count"] = count self.pre_get_list(response) ``` `pre_get_list` runs *after* the dump and receives the serialized response, not the model list, so priming there would happen once every per-row `localized_name` lookup had already gone to the hook. Despite the name it is a response mutator, not a pre-query hook. There is no clean seam for this: the list schema is built per-request by `model2schemaconverter.convert(pruned_select_cols)` whenever `select_columns` is present — which the React list views always send — so a custom schema with `@pre_dump(pass_many=True)` is bypassed, and `BaseSupersetModelRestApi.get_list_headless` delegates to `super()` without ever seeing `lst`. The remaining options are monkeypatching `datamodel.query` per request on a shared instance, or threading an i18n concern through `marshmallow_compatibility`; both seem worse than the problem. So this stays a documented limitation — the docs tell implementers to back their hook with a cache or in-process map for that path. Happy to revisit if a maintainer sees a seam I've missed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
