Skip to content

FIREFLY-2076: Fix charts not showing for table/chart IDs containing a period #2054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jaladh-singhal merged 2 commits into from
Oct 29, 2026
Merged

FIREFLY-2076: Fix charts not showing for table/chart IDs containing a period #2054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fbae9be
FIREFLY-2076: Use array paths for chart and table-stats IDs
jaladh-singhal Oct 27, 2026
1c56645
FIREFLY-2076: Replace lodash get/set with native syntax in chart redu…
jaladh-singhal Oct 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 21 additions & 20 deletions src/firefly/js/charts/ChartsCntlr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* License information at https://github.com/Caltech-IPAC/firefly/blob/master/License.txt
*/

import {cloneDeep, has, get, isArray, isEmpty, isString, isUndefined, omit, omitBy, set, range, unset, uniqueId} from 'lodash';
import {cloneDeep, get, isArray, isEmpty, isString, isUndefined, omit, omitBy, set, range, unset, uniqueId} from 'lodash';
import shallowequal from 'shallowequal';

import {flux} from '../core/ReduxFlux.js';
Expand Down Expand Up @@ -700,35 +700,36 @@ function reduceData(state={}, action={}) {
{
const {chartId, chartType, mounted, ...rest} = action.payload;
// if a chart is replaced (added with the same id) mounted should not change
const nMounted = isUndefined(mounted) ? get(state, [chartId, 'mounted']) : mounted;
const nMounted = isUndefined(mounted) ? state[chartId]?.mounted : mounted;
// save the original payload, so that the chart could be recreated
rest['_original'] = cloneDeep(action.payload);
applyDefaults(rest);
useScatterGL && changeToScatterGL(rest);

// the first trace is put as the last curve for plotly rendering
const tData = get(rest, ['data', 'length']);
const tData = rest.data?.length;
if (tData) {
Object.assign(rest, {activeTrace: tData-1, curveNumberMap: range(tData)});
}
state = updateSet(state, chartId,
omitBy({
state = {...state,
[chartId]: omitBy({
chartId,
chartType,
mounted: nMounted,
...rest
}, isUndefined));
}, isUndefined)};
logger.info(`CHART_ADD ${chartId} #mounted ${nMounted}`);
return state;
}
case (CHART_UPDATE) :
{
const {chartId, changes} = action.payload;
let chartData = getChartData(chartId);
let chartData = state[chartId] ?? {};
chartData = updateObject(chartData, changes);
useScatterGL && changeToScatterGL(chartData);

return updateSet(state, chartId, chartData);
// updateObject returns the same object when nothing changed; keep state too, so subscribers don't re-render
return chartData === state[chartId] ? state : {...state, [chartId]: chartData};
}
case (CHART_TRACE_REMOVE) :
{
Expand All @@ -743,30 +744,30 @@ function reduceData(state={}, action={}) {
chartData = updateObject(chartData, moreChanges);
}
}
return updateSet(state, chartId, chartData);
return chartData === state[chartId] ? state : {...state, [chartId]: chartData};
}
case (CHART_REMOVE) :
{
const {chartId} = action.payload;
logger.info('CHART_REMOVE '+chartId);
return omit(state, chartId);
return omit(state, [chartId]);
}
case (CHART_MOUNTED) :
{
const {chartId} = action.payload;
const n = get(state, [chartId,'mounted'], 0);
state = updateSet(state, [chartId,'mounted'], Number(n) + 1);
const mountedCnt = state[chartId]?.mounted ?? 0;
state = {...state, [chartId]: {...state[chartId], mounted: Number(mountedCnt) + 1}};
logger.info(`CHART_MOUNTED ${chartId} #mounted ${state[chartId].mounted}`);

return state;
}
case (CHART_UNMOUNTED) :
{
const {chartId} = action.payload;
if (has(state, chartId)) {
const n = get(state, [chartId,'mounted'], 0);
if (n > 0) {
state = updateSet(state, [chartId, 'mounted'], Number(n) - 1);
if (state[chartId]) {
const mountedCnt = state[chartId].mounted ?? 0;
if (mountedCnt > 0) {
state = {...state, [chartId]: {...state[chartId], mounted: Number(mountedCnt) - 1}};
} else {
logger.error(`CHART_UNMOUNT on unmounted chartId ${chartId}`);
}
Expand Down Expand Up @@ -934,7 +935,7 @@ function removeTrace({chartId, traceNum}) {
}

export function getChartData(chartId, defaultChartData={}) {
return get(flux.getState(), [CHART_SPACE_PATH, 'data', chartId], defaultChartData);
return flux.getState()[CHART_SPACE_PATH]?.data?.[chartId] ?? defaultChartData;
}


Expand Down Expand Up @@ -1003,7 +1004,7 @@ export function getExpandedChartProps() {

export function getChartIdsForTable(tbl_id) {
const chartIds = [];
const state = get(flux.getState(), [CHART_SPACE_PATH, 'data']);
const state = flux.getState()[CHART_SPACE_PATH]?.data ?? {};
Object.keys(state).forEach((cid) => {
if (state[cid].tbl_id === tbl_id) {
chartIds.push(cid);
Expand All @@ -1014,7 +1015,7 @@ export function getChartIdsForTable(tbl_id) {

export function getChartIdsInGroup(groupId) {
const chartIds = [];
const state = get(flux.getState(), [CHART_SPACE_PATH, 'data']);
const state = flux.getState()[CHART_SPACE_PATH]?.data ?? {};
Object.keys(state).forEach((cid) => {
if (state[cid].groupId === groupId) {
chartIds.push(cid);
Expand All @@ -1024,7 +1025,7 @@ export function getChartIdsInGroup(groupId) {
}

export function removeChartsInGroup(groupId) {
const chartData = get(flux.getState(), [CHART_SPACE_PATH, 'data']);
const chartData = flux.getState()[CHART_SPACE_PATH]?.data ?? {};
Object.values(chartData)
.filter( (v) => !groupId || v.groupId === groupId)
.forEach( (v) => dispatchChartRemove(v.chartId));
Expand Down
33 changes: 16 additions & 17 deletions src/firefly/js/charts/TableStatsCntlr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import {useEffect} from 'react';
import {flux} from '../core/ReduxFlux.js';

import {get, has, omit} from 'lodash';
import {omit} from 'lodash';

import {updateSet, updateMerge} from '../util/WebUtil.js';
import ColValuesStatistics from './ColValuesStatistics.js';
import {REINIT_APP} from '../core/CoreConst';

Expand Down Expand Up @@ -54,11 +53,11 @@ function reducers() {
* @param {function} dispatcher only for special dispatching uses such as remote
*/
export function dispatchLoadTblStats(searchRequest, dispatcher= flux.process) {
const tbl_id = get(searchRequest, 'tbl_id');
const tbl_id = searchRequest?.tbl_id;
if (!tbl_id) return;
// use resultSetID to determine if a call needs to be placed
const {resultSetID, numericColCnt} = get(flux.getState(), [TBLSTATS_DATA_KEY, tbl_id], {});
const resultSetIDNow = get(getTblById(tbl_id), 'tableMeta.resultSetID');
const {resultSetID, numericColCnt} = flux.getState()[TBLSTATS_DATA_KEY]?.[tbl_id] ?? {};
const resultSetIDNow = getTblById(tbl_id)?.tableMeta?.resultSetID;
const curNumericColCnt = getColumns(getTblById(tbl_id), COL_TYPE.NUMBER)?.length;

if (resultSetID !== resultSetIDNow
Expand Down Expand Up @@ -103,24 +102,24 @@ export function reducer(state=getInitState(), action={}) {
case (TablesCntlr.TABLE_REMOVE) :
{
const {tbl_id} = action.payload;
if (has(state, tbl_id)) {
return Object.assign({}, omit(state,[tbl_id]));
if (state[tbl_id]) {
return omit(state, [tbl_id]);
}
return state;
}
case (LOAD_TBL_STATS) :
{
const tblId = get(action.payload, ['searchRequest', 'tbl_id']);
const tblId = action.payload.searchRequest?.tbl_id;

// save original table file path
const resultSetID = get(getTblById(tblId), 'tableMeta.resultSetID');
const resultSetID = getTblById(tblId)?.tableMeta?.resultSetID;

return updateSet(state, tblId, {resultSetID, isColStatsReady: false});
return {...state, [tblId]: {resultSetID, isColStatsReady: false}};
}
case (UPDATE_TBL_STATS) :
{
const {tblId, isColStatsReady, colStats} = action.payload;
return updateMerge(state, tblId, {isColStatsReady, colStats});
return {...state, [tblId]: {...state[tblId], isColStatsReady, colStats}};
}
case (REINIT_APP) :
return getInitState();
Expand Down Expand Up @@ -149,7 +148,7 @@ function fetchTblStats(dispatch, activeTableServerRequest) {

const {tbl_id} = activeTableServerRequest;

const resultSetIDOnSubmit = get(getTblById(tbl_id), 'tableMeta.resultSetID');
const resultSetIDOnSubmit = getTblById(tbl_id)?.tableMeta?.resultSetID;

// searchRequest
const sreq = cloneRequest(activeTableServerRequest, {'startIdx': 0, 'pageSize': MAX_ROW});
Expand All @@ -162,15 +161,15 @@ function fetchTblStats(dispatch, activeTableServerRequest) {
(tableModel) => {

// if the original table has changed no need to continue
const resultSetIDOnFetch = get(getTblById(tbl_id), 'tableMeta.resultSetID');
const resultSetIDOnFetch = getTblById(tbl_id)?.tableMeta?.resultSetID;
if (resultSetIDOnSubmit !== resultSetIDOnFetch) {
return;
}

const tblData = get(tableModel, 'tableData.data');
const tblData = tableModel?.tableData?.data;
let colStats = [];
if (tblData) {
const columns = get(getTblById(tbl_id), 'tableData.columns', []);
const columns = getTblById(tbl_id)?.tableData?.columns ?? [];
colStats = tblData.reduce((colstats, arow) => {
const r = new ColValuesStatistics(...arow);
const col = columns.find((c)=>{return c.name=== r.name;});
Expand Down Expand Up @@ -206,7 +205,7 @@ function fetchTblStats(dispatch, activeTableServerRequest) {
).finally(() => {
pendingStatsTblIds.delete(tbl_id);
const currentRequest = getTblById(tbl_id)?.request;
const currentResultSetID = get(getTblById(tbl_id), 'tableMeta.resultSetID');
const currentResultSetID = getTblById(tbl_id)?.tableMeta?.resultSetID;
if (currentRequest && currentResultSetID !== resultSetIDOnSubmit) {
dispatchLoadTblStats(currentRequest);
}
Expand All @@ -216,7 +215,7 @@ function fetchTblStats(dispatch, activeTableServerRequest) {
export function getColValStats(tblId) {
if (!tblId) { return undefined; }
const tblStatsData = flux.getState()[TBLSTATS_DATA_KEY][tblId];
if (get(tblStatsData,'isColStatsReady')) {
if (tblStatsData?.isColStatsReady) {
return tblStatsData.colStats;
}
}
Expand Down
63 changes: 63 additions & 0 deletions src/firefly/js/charts/__tests__/ChartsCntlr-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*eslint-env node, jest */
import {flux} from 'firefly/core/ReduxFlux.js';
import {
CHART_ADD, CHART_MOUNTED, CHART_REMOVE, CHART_TRACE_REMOVE, CHART_UNMOUNTED, CHART_UPDATE, reducer
} from 'firefly/charts/ChartsCntlr.js';

// default chart ids are 'default-' + tbl_id, and tbl_id can contain a period
const DOTTED_CHART_ID = 'default-2mass.catalog';

describe('ChartsCntlr reducer with a chart id containing a period', () => {
const addedState = reducer(undefined,
{type: CHART_ADD, payload: {chartId: DOTTED_CHART_ID, chartType: 'plot.ly', groupId: 'main'}});

test('CHART_ADD stores the chart under the whole id, not a nested path', () => {
expect(addedState.data[DOTTED_CHART_ID]?.chartId).toBe(DOTTED_CHART_ID);
expect(addedState.data['default-2mass']).toBeUndefined();
});

test('CHART_MOUNTED and CHART_UNMOUNTED update the mounted count of that chart', () => {
const mountedState = reducer(addedState, {type: CHART_MOUNTED, payload: {chartId: DOTTED_CHART_ID}});
expect(mountedState.data[DOTTED_CHART_ID].mounted).toBe(1);

const unmountedState = reducer(mountedState, {type: CHART_UNMOUNTED, payload: {chartId: DOTTED_CHART_ID}});
expect(unmountedState.data[DOTTED_CHART_ID].mounted).toBe(0);
});

test('CHART_REMOVE removes the chart', () => {
const removedState = reducer(addedState, {type: CHART_REMOVE, payload: {chartId: DOTTED_CHART_ID}});
expect(removedState.data).toEqual({});
});

test('CHART_UPDATE applies changes to that chart', () => {
const updatedState = reducer(addedState,
{type: CHART_UPDATE, payload: {chartId: DOTTED_CHART_ID, changes: {'layout.title.text': 'J vs H-K'}}});
expect(updatedState.data[DOTTED_CHART_ID].layout.title.text).toBe('J vs H-K');
expect(updatedState.data['default-2mass']).toBeUndefined();
});

test('CHART_UPDATE returns the same state when changes do not change the chart', () => {
const chartType = addedState.data[DOTTED_CHART_ID].chartType;
const unchangedState = reducer(addedState,
{type: CHART_UPDATE, payload: {chartId: DOTTED_CHART_ID, changes: {chartType}}});
expect(unchangedState.data).toBe(addedState.data);
});
});

// CHART_TRACE_REMOVE reads the current chart from the store (removeTrace calls getChartData), not from the reducer state
describe('ChartsCntlr reducer with a chart id containing a period, reading chart data from the store', () => {
let storeChartState;
beforeEach(() => jest.spyOn(flux, 'getState').mockImplementation(() => ({charts: storeChartState})));
afterEach(() => jest.restoreAllMocks());

test('CHART_TRACE_REMOVE removes the trace from that chart', () => {
const twoTraces = [{name: 'trace0', type: 'scatter'}, {name: 'trace1', type: 'scatter'}];
storeChartState = reducer(undefined,
{type: CHART_ADD, payload: {chartId: DOTTED_CHART_ID, chartType: 'plot.ly', groupId: 'main', data: twoTraces}});

const traceRemovedState = reducer(storeChartState,
{type: CHART_TRACE_REMOVE, payload: {chartId: DOTTED_CHART_ID, traceNum: 0}});
expect(traceRemovedState.data[DOTTED_CHART_ID].data.map((trace) => trace.name)).toEqual(['trace1']);
expect(traceRemovedState.data['default-2mass']).toBeUndefined();
});
});
21 changes: 21 additions & 0 deletions src/firefly/js/charts/__tests__/TableStatsCntlr-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*eslint-env node, jest */
import {reducer, UPDATE_TBL_STATS} from 'firefly/charts/TableStatsCntlr.js';
import {TABLE_REMOVE} from 'firefly/tables/TablesCntlr.js';

const DOTTED_TBL_ID = '2mass.catalog';

describe('TableStatsCntlr reducer with a table id containing a period', () => {
const colStats = [{name: 'ra', min: 202.3, max: 202.7}];
const updatedState = reducer({},
{type: UPDATE_TBL_STATS, payload: {tblId: DOTTED_TBL_ID, isColStatsReady: true, colStats}});

test('UPDATE_TBL_STATS stores stats under the whole id, not a nested path', () => {
expect(updatedState[DOTTED_TBL_ID]).toEqual({isColStatsReady: true, colStats});
expect(updatedState['2mass']).toBeUndefined();
});

test('TABLE_REMOVE removes the stats of that table', () => {
const removedState = reducer(updatedState, {type: TABLE_REMOVE, payload: {tbl_id: DOTTED_TBL_ID}});
expect(removedState).toEqual({});
});
});
2 changes: 1 addition & 1 deletion src/firefly/js/charts/ui/PlotlyWrapper.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ function isSlowResize() {
export async function downloadChart(chartId) {
const Plotly= await getPlotLy();
// in API we can have same chart unexpanded and expanded
const chartDivAll = document.querySelectorAll(`#${chartId}`);
const chartDivAll = document.querySelectorAll(`#${CSS.escape(chartId)}`);
if (chartId && chartDivAll && chartDivAll.length > 0) {
const chartDiv = chartDivAll[chartDivAll.length-1];
showPlotLySaveDialog(Plotly,chartDiv);
Expand Down
Loading