rusackas commented on PR #39925: URL: https://github.com/apache/superset/pull/39925#issuecomment-4724436006
Bot findings of interest: Whining that it's too big: > If any one piece regresses in production, you can't revert without losing all of it. At minimum, peel off: (a) the i18n catalog churn — it has no business in a URL refactor and will conflict-thrash; (b) the navigationUtils helper + call-site migration (the genuinely valuable, low-risk core); (c) the backend middleware/PWA/redirect pieces; (d) the response-normalizer. ^ I might agree with the translations, if that even CAN be taken out cleanly. If not, whatever. > Dangling references to PLAN.md and internal audit scaffolding in committed code. Comments reference PLAN.md (3×, and the file is not in the PR), plus ~125 references to an internal methodology — AF-1 (26×), AF-2, RF-2, Slice 7, review-fix, C5 in PR, dated 2026-06-01 tags. Future maintainers can't decode any of it. Keep the actual rationale in the comments; strip the slice/audit-ID/PLAN.md vocabulary. This is a cleanliness blocker for a permanent codebase. Other random things that sound problematic, perhaps: > The response normalizer is heavy machinery for one field. NORMALIZED_URL_FIELDS contains exactly explore_url. To strip that one field it deep-walks every JSON response (every chart-data payload, every large json_metadata) under subdir deployments, with a depth-100 cap and WeakSet cycle detection that is dead code for freshly-JSON.parsed data (parsed JSON can't contain cycles or shared refs). The shared-reference branch is also subtly inconsistent (first occurrence normalized, second returned raw) — harmless only because parsed JSON has no shared refs. For one field, a targeted strip at the few known call sites would be dramatically cheaper and simpler. I'd push back on landing the generic walker. > SupersetClient.getUrl semantic change is a public-API contract change. Third-party plugins consume this. Auto-deduping a leading appRoot is a reasonable safety net, but it changes published behavior and can mask genuine /superset/superset/... routes. Needs a deliberate call-out in UPDATING.md (the permalink flag is documented there; this isn't clearly). LMK what you think... we can take it from there. -- 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]
