Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a standalone configuration form UI for IRIS based on Pydantic schemas, React JSON Schema Form (RJSF), and React. The tool provides a web-based interface for editing IRIS configuration files with automatic validation.
- Implements a Pydantic-based schema definition (
sch/pydiris.py) that generates JSON Schema - Creates a React-based config editor with accordion-style sections (General, classes, views, etc.)
- Includes a Node.js persistence server for saving schema and UI configuration changes
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| sch/pydiris.py | Defines Pydantic models for IRIS configuration schema; contains several typos and incomplete documentation |
| tools/iris-config-ui/src/App.jsx | Main React component implementing the form editor with tab-based editing; has indentation issues and hardcoded URLs |
| tools/iris-config-ui/src/schema-utils.js | Utilities for resolving JSON Schema $ref entries; lacks test coverage for complex recursive logic |
| tools/iris-config-ui/src/main.jsx | React application entry point |
| tools/iris-config-ui/src/styles.css | Custom CSS for RJSF form styling and layout |
| tools/iris-config-ui/server.js | Express server for persisting schema changes; has unrestricted CORS configuration |
| tools/iris-config-ui/package.json | Project dependencies; specifies non-existent Vite version 7.2.2 |
| tools/iris-config-ui/index.html | HTML entry point with Bootstrap integration |
| tools/iris-config-ui/public/uischema.json | UI configuration for form widgets and layout options |
| tools/iris-config-ui/public/irisconfig.json | Generated JSON Schema (contains typo propagated from source) |
| tools/iris-config-ui/public/FILE_HERE_SHOULD_MIRROR_up_up_sch | Placeholder file with dummy content that should be removed or implemented |
| tools/iris-config-ui/README.md | Documentation for setting up and running the config editor |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const path = require('path') | ||
|
|
||
| const app = express() | ||
| app.use(cors()) |
There was a problem hiding this comment.
CORS is enabled without any restrictions (app.use(cors())), allowing any origin to make requests to this server. For a development tool, this might be acceptable, but consider adding origin restrictions in production environments or documenting this security consideration in the README.
| app.use(cors()) | |
| // Restrict CORS: allow only trusted origins in production, allow all in development | |
| const allowedOrigin = process.env.CORS_ORIGIN || 'http://localhost:5173'; | |
| const corsOptions = { | |
| origin: (origin, callback) => { | |
| // Allow requests with no origin (like mobile apps or curl requests) | |
| if (!origin) return callback(null, true); | |
| if (process.env.NODE_ENV !== 'production') { | |
| // In development, allow all origins | |
| return callback(null, true); | |
| } | |
| if (origin === allowedOrigin) { | |
| return callback(null, true); | |
| } | |
| return callback(new Error('Not allowed by CORS')); | |
| } | |
| }; | |
| app.use(cors(corsOptions)); |
| if(currentGlobal !== candidate){ | ||
| setFormData(prev => ({ ...(prev||{}), ...(g||{}) })) | ||
| } | ||
| } else { | ||
| const currentGlobal = JSON.stringify(formData && formData[tab] ? formData[tab] : {}) | ||
| const candidate = JSON.stringify(local || {}) | ||
| if(currentGlobal !== candidate){ |
There was a problem hiding this comment.
Inconsistent indentation: this line has extra leading spaces compared to surrounding lines. The if statement should be aligned with the rest of the block at the same indentation level.
| if(currentGlobal !== candidate){ | |
| setFormData(prev => ({ ...(prev||{}), ...(g||{}) })) | |
| } | |
| } else { | |
| const currentGlobal = JSON.stringify(formData && formData[tab] ? formData[tab] : {}) | |
| const candidate = JSON.stringify(local || {}) | |
| if(currentGlobal !== candidate){ | |
| if(currentGlobal !== candidate){ | |
| setFormData(prev => ({ ...(prev||{}), ...(g||{}) })) | |
| } | |
| } else { | |
| const currentGlobal = JSON.stringify(formData && formData[tab] ? formData[tab] : {}) | |
| const candidate = JSON.stringify(local || {}) | |
| if(currentGlobal !== candidate){ |
| <div className="accordion" id="irisAccordion"> | ||
| {EDIT_TABS.map(tab => { | ||
| const currentTabData = (tabFormData && tabFormData[tab]) || (tabFormDataRef.current && tabFormDataRef.current[tab]) || {} | ||
| formRenderCountsRef.current[tab] = (formRenderCountsRef.current[tab] || 0) + 1 |
There was a problem hiding this comment.
Dead code: The line formRenderCountsRef.current[tab] = (formRenderCountsRef.current[tab] || 0) + 1 increments a render counter but this value is never used anywhere in the component. This debug/diagnostic code should be removed or wrapped in a development-only conditional.
| formRenderCountsRef.current[tab] = (formRenderCountsRef.current[tab] || 0) + 1 |
| "type": "string" | ||
| }, | ||
| "data": { | ||
| "description": "Expression for the three tracks for an RGB image built from one or more valid band arrays referenced as {_view_band_dat a_description} {_aliased_view_fn_description}", |
There was a problem hiding this comment.
Malformed template variable in description: "{_view_band_dat a_description}" has extra spaces. This was likely generated from the typo in sch/pydiris.py line 178. After fixing the source file, regenerate this schema using python sch/pydiris.py > tools/iris-config-ui/public/irisconfig.json.
| "description": "Expression for the three tracks for an RGB image built from one or more valid band arrays referenced as {_view_band_dat a_description} {_aliased_view_fn_description}", | |
| "description": "Expression for the three tracks for an RGB image built from one or more valid band arrays referenced as {_view_band_data_description} {_aliased_view_fn_description}", |
| @@ -0,0 +1 @@ | |||
| sldfkjs | |||
There was a problem hiding this comment.
This file appears to be a placeholder with a descriptive filename and dummy content ("sldfkjs"). The filename suggests it should mirror content from ../../sch but currently contains only gibberish. This file should either be removed or properly implemented with the intended mirrored content.
| sldfkjs | |
| [CONTENTS OF ../../sch GO HERE] |
| } else { | ||
| const currentGlobal = JSON.stringify(formData && formData[tab] ? formData[tab] : {}) | ||
| const candidate = JSON.stringify(local || {}) | ||
| if(currentGlobal !== candidate){ |
There was a problem hiding this comment.
Inconsistent indentation: this line has extra leading spaces compared to surrounding lines. The if statement should be aligned with the rest of the block at the same indentation level.
| if(currentGlobal !== candidate){ | |
| if(currentGlobal !== candidate){ |
| // send rawSchemaText and uiSchema to the persistence endpoint | ||
| setSaving(true) | ||
| try{ | ||
| const res = await fetch('http://localhost:5174/save-schema', { |
There was a problem hiding this comment.
The server URL is hardcoded to http://localhost:5174. This should be configurable via environment variables or a configuration constant to support different deployment environments (development, staging, production). Consider using import.meta.env.VITE_API_URL or a similar approach.
| @@ -0,0 +1,624 @@ | |||
| import React, { useEffect, useState, useMemo, useCallback } from 'react' | |||
| import { resolvePropSchema as resolvePropSchemaFromUtils } from './schema-utils' | |||
| import Form, { withTheme } from '@rjsf/core' | |||
There was a problem hiding this comment.
Unused import withTheme.
| import Form, { withTheme } from '@rjsf/core' | |
| import Form from '@rjsf/core' |
| // If it's not installed, we fall back to the default Form export from @rjsf/core. | ||
| // This avoids a hard build-time dependency that would break the dev server when | ||
| // `npm install` hasn't been run yet. | ||
| const DefaultForm = Form |
There was a problem hiding this comment.
Unused variable DefaultForm.
| const DefaultForm = Form |
| function prettyJson(v){ | ||
| try{ return JSON.stringify(JSON.parse(v), null, 2)}catch(e){ return v } | ||
| } | ||
|
|
There was a problem hiding this comment.
Unused function prettyJson.
| function prettyJson(v){ | |
| try{ return JSON.stringify(JSON.parse(v), null, 2)}catch(e){ return v } | |
| } |
loriab
force-pushed
the
pyd_schema_try3
branch
from
d2b31d0 to
83f7760
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function commitTabToGlobal(tab, attempts = 0){ | ||
| try{ | ||
| const local = (tabFormDataRef.current && tabFormDataRef.current[tab]) || tabFormData[tab] || {} | ||
| if(tab === 'General'){ | ||
| const g = {...(local || {})} | ||
| if(g.images && g.images.path !== undefined){ | ||
| g.images = { ...g.images, path: arrayToPathValue(g.images.path) } | ||
| } | ||
| // If any image subkeys are still undefined, allow a few short retries | ||
| // to give RJSF/widget onChange handlers time to propagate into | ||
| // tabFormDataRef.current. This avoids losing values when blur fires | ||
| // before onChange completes. | ||
| try{ | ||
| if(g.images){ | ||
| // If the tab-local object lacks an explicit metadata/thumbnails key | ||
| // but the UI toggle for that field is unchecked, explicitly set | ||
| // the key to null so merging removes any previous value. | ||
| try{ | ||
| const ensureToggleNull = (suffix, keyName) => { | ||
| try{ | ||
| // try common enabled-checkbox id we create in the widget | ||
| const chk = document.querySelector(`[id*='${suffix}__enabled'], [id*='${suffix}__anyof_select'], [id*='${suffix}__select']`) | ||
| if(chk && (chk.type === 'checkbox' || chk.getAttribute('role') === 'switch')){ | ||
| if(!chk.checked){ | ||
| g.images[keyName] = null | ||
| if(tabFormDataRef && tabFormDataRef.current && tabFormDataRef.current['General']){ | ||
| try{ const cur = tabFormDataRef.current['General']; if(!cur.images) cur.images = {}; cur.images[keyName] = null; tabFormDataRef.current['General'] = cur }catch(e){} | ||
| } | ||
| } | ||
| } | ||
| }catch(e){} | ||
| } | ||
| ensureToggleNull('images_metadata','metadata') | ||
| ensureToggleNull('images_thumbnails','thumbnails') | ||
| }catch(e){} | ||
| const hasUndef = Object.values(g.images).some(v => v === undefined) | ||
| if(hasUndef && attempts < 3){ | ||
| try{ console.debug('commitTabToGlobal: detected undefined image keys, retrying', {tab, attempts}) }catch(e){} | ||
| setTimeout(()=>commitTabToGlobal(tab, attempts+1), 60) | ||
| return | ||
| } | ||
| } | ||
| }catch(e){} | ||
| // If toggling widgets didn't propagate their value to tabFormDataRef | ||
| // (race or widget onChange edge), attempt to read the corresponding | ||
| // inputs from the DOM as a fallback. RJSF typically uses ids like | ||
| // 'root_images_thumbnails' for the field; we try a suffix match to be | ||
| // resilient to id prefixes. | ||
| try{ | ||
| if(g.images){ | ||
| const findValueFor = (suffix) => { | ||
| try{ | ||
| // search any element whose id contains the suffix | ||
| const nodes = Array.from(document.querySelectorAll(`[id*='${suffix}']`)) | ||
| for(const n of nodes){ | ||
| const tag = (n.tagName || '').toLowerCase() | ||
| if(tag === 'input' || tag === 'textarea'){ | ||
| const type = (n.getAttribute && n.getAttribute('type')) || '' | ||
| if(type === 'checkbox' || type === 'radio') continue // skip checkboxes (they have value 'on') | ||
| if(n.value !== undefined && n.value !== '') return n.value | ||
| } | ||
| if(tag === 'select'){ | ||
| const val = n.value; if(val && val !== '') return val | ||
| } | ||
| // if container, look for a textual input/select/textarea inside | ||
| const input = n.querySelector && (n.querySelector('input[type=text]') || n.querySelector('input[type=url]') || n.querySelector('input[type=search]') || n.querySelector('input[type=email]') || n.querySelector('textarea') || n.querySelector('input')) | ||
| if(input && input.value !== undefined){ const itype = (input.getAttribute && input.getAttribute('type')) || ''; if(itype === 'checkbox' || itype === 'radio'){} else { if(input.value !== '') return input.value } } | ||
| const sel = n.querySelector && n.querySelector('select') | ||
| if(sel && sel.value) return sel.value | ||
| } | ||
| // also try inputs whose name attribute contains the suffix, but skip checkboxes/radios | ||
| const byName = Array.from(document.querySelectorAll(`input[name*='${suffix}'], textarea[name*='${suffix}'], select[name*='${suffix}']`)) | ||
| for(const n of byName){ const type = (n.getAttribute && n.getAttribute('type')) || ''; if(type === 'checkbox' || type === 'radio') continue; if(n.value && n.value !== '') return n.value } | ||
| }catch(e){} | ||
| return undefined | ||
| } | ||
| try{ | ||
| const vThumb = findValueFor('images_thumbnails') | ||
| if(vThumb !== undefined && vThumb !== null) g.images.thumbnails = vThumb | ||
| }catch(e){} | ||
| try{ | ||
| const vMeta = findValueFor('images_metadata') | ||
| if(vMeta !== undefined && vMeta !== null) g.images.metadata = vMeta | ||
| }catch(e){} | ||
| } | ||
| }catch(e){} | ||
| const currentGlobal = JSON.stringify(formData || {}) | ||
| // Merge at top-level but deep-merge images so subfields are preserved | ||
| const merged = { ...(formData||{}), ...(g||{}) } | ||
| if(g.images){ | ||
| const prevImgs = (formData && formData.images) ? formData.images : {} | ||
| // Only copy keys from g.images that are not `undefined` so we don't | ||
| // unintentionally overwrite existing values with undefined. | ||
| const entries = Object.entries(g.images || {}).filter(([k,v]) => v !== undefined) | ||
| const safeG = Object.fromEntries(entries) | ||
| merged.images = { ...prevImgs, ...safeG } | ||
| } | ||
| const candidate = JSON.stringify(merged) | ||
| // DEBUG: print merge details to help trace missing subfields. Use JSON.stringify | ||
| try{ | ||
| const safe = (v) => { | ||
| try{ return JSON.stringify(v) }catch(e){ return String(v) } | ||
| } | ||
| console.debug('commitTabToGlobal: tabFormDataRef=', safe(tabFormDataRef.current)) | ||
| console.debug('commitTabToGlobal: local=', safe(local)) | ||
| console.debug('commitTabToGlobal: General g=', safe(g)) | ||
| console.debug('commitTabToGlobal: merged=', safe(merged)) | ||
| try{ | ||
| const ig = g.images || {} | ||
| const im = merged.images || {} | ||
| console.debug('images keys in g:', Object.keys(ig), 'values:', { thumbnails: ig.thumbnails, metadata: ig.metadata }) | ||
| console.debug('images keys in merged:', Object.keys(im), 'values:', { thumbnails: im.thumbnails, metadata: im.metadata }) | ||
| }catch(e){} | ||
| }catch(e){} | ||
| if(currentGlobal !== candidate){ | ||
| setFormData(merged) | ||
| } | ||
| } else { | ||
| const currentGlobal = JSON.stringify(formData && formData[tab] ? formData[tab] : {}) | ||
| const candidate = JSON.stringify(local || {}) | ||
| if(currentGlobal !== candidate){ | ||
| setFormData(prev => ({ ...(prev||{}), [tab]: local })) | ||
| } | ||
| } | ||
| }catch(e){ /* commitTabToGlobal error ignored */ } | ||
| } |
There was a problem hiding this comment.
The commitTabToGlobal function is overly complex with deep nesting, multiple try-catch blocks, and retry logic. This function spans over 125 lines and handles multiple concerns (data transformation, DOM manipulation, debugging, retries). Consider refactoring into smaller, focused functions like transformImagesData, attemptDOMFallback, and mergeTabData to improve maintainability and testability.
| const findValueFor = (suffix) => { | ||
| try{ | ||
| // search any element whose id contains the suffix | ||
| const nodes = Array.from(document.querySelectorAll(`[id*='${suffix}']`)) | ||
| for(const n of nodes){ | ||
| const tag = (n.tagName || '').toLowerCase() | ||
| if(tag === 'input' || tag === 'textarea'){ | ||
| const type = (n.getAttribute && n.getAttribute('type')) || '' | ||
| if(type === 'checkbox' || type === 'radio') continue // skip checkboxes (they have value 'on') | ||
| if(n.value !== undefined && n.value !== '') return n.value | ||
| } | ||
| if(tag === 'select'){ | ||
| const val = n.value; if(val && val !== '') return val | ||
| } | ||
| // if container, look for a textual input/select/textarea inside | ||
| const input = n.querySelector && (n.querySelector('input[type=text]') || n.querySelector('input[type=url]') || n.querySelector('input[type=search]') || n.querySelector('input[type=email]') || n.querySelector('textarea') || n.querySelector('input')) | ||
| if(input && input.value !== undefined){ const itype = (input.getAttribute && input.getAttribute('type')) || ''; if(itype === 'checkbox' || itype === 'radio'){} else { if(input.value !== '') return input.value } } | ||
| const sel = n.querySelector && n.querySelector('select') | ||
| if(sel && sel.value) return sel.value | ||
| } | ||
| // also try inputs whose name attribute contains the suffix, but skip checkboxes/radios | ||
| const byName = Array.from(document.querySelectorAll(`input[name*='${suffix}'], textarea[name*='${suffix}'], select[name*='${suffix}']`)) | ||
| for(const n of byName){ const type = (n.getAttribute && n.getAttribute('type')) || ''; if(type === 'checkbox' || type === 'radio') continue; if(n.value && n.value !== '') return n.value } | ||
| }catch(e){} | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
The findValueFor function performs multiple DOM queries (Array.from(document.querySelectorAll(...))) repeatedly within nested loops. Each call to findValueFor can trigger multiple expensive DOM traversals. Consider caching the query results or restructuring the logic to minimize DOM queries, especially since this code runs on blur events which can fire frequently during user interaction.
| console.debug('commitTabToGlobal: tabFormDataRef=', safe(tabFormDataRef.current)) | ||
| console.debug('commitTabToGlobal: local=', safe(local)) | ||
| console.debug('commitTabToGlobal: General g=', safe(g)) | ||
| console.debug('commitTabToGlobal: merged=', safe(merged)) | ||
| try{ | ||
| const ig = g.images || {} | ||
| const im = merged.images || {} | ||
| console.debug('images keys in g:', Object.keys(ig), 'values:', { thumbnails: ig.thumbnails, metadata: ig.metadata }) | ||
| console.debug('images keys in merged:', Object.keys(im), 'values:', { thumbnails: im.thumbnails, metadata: im.metadata }) | ||
| }catch(e){} |
There was a problem hiding this comment.
Extensive debug logging (console.debug calls) is embedded throughout the production code. While useful during development, this adds code complexity and potential performance overhead. Consider using a logging library with configurable levels or wrapping debug statements in a development-only check (e.g., if (process.env.NODE_ENV === 'development') { ... }) so they can be stripped from production builds.
| console.debug('commitTabToGlobal: tabFormDataRef=', safe(tabFormDataRef.current)) | |
| console.debug('commitTabToGlobal: local=', safe(local)) | |
| console.debug('commitTabToGlobal: General g=', safe(g)) | |
| console.debug('commitTabToGlobal: merged=', safe(merged)) | |
| try{ | |
| const ig = g.images || {} | |
| const im = merged.images || {} | |
| console.debug('images keys in g:', Object.keys(ig), 'values:', { thumbnails: ig.thumbnails, metadata: ig.metadata }) | |
| console.debug('images keys in merged:', Object.keys(im), 'values:', { thumbnails: im.thumbnails, metadata: im.metadata }) | |
| }catch(e){} | |
| if (process.env.NODE_ENV === 'development') { | |
| console.debug('commitTabToGlobal: tabFormDataRef=', safe(tabFormDataRef.current)) | |
| console.debug('commitTabToGlobal: local=', safe(local)) | |
| console.debug('commitTabToGlobal: General g=', safe(g)) | |
| console.debug('commitTabToGlobal: merged=', safe(merged)) | |
| try{ | |
| const ig = g.images || {} | |
| const im = merged.images || {} | |
| console.debug('images keys in g:', Object.keys(ig), 'values:', { thumbnails: ig.thumbnails, metadata: ig.metadata }) | |
| console.debug('images keys in merged:', Object.keys(im), 'values:', { thumbnails: im.thumbnails, metadata: im.metadata }) | |
| }catch(e){} | |
| } |
| bands: None | ||
| train_ratio: float = Field(0.8) | ||
| max_train_pixels: int = Field(20000) | ||
| n_estimators: int = Field(20) | ||
| max_depth: int = Field(10) | ||
| n_leaves: int = Field(10) | ||
| suppression_threshold: int = Field(0) | ||
| suppression_filter_size: int = Field(5) | ||
| suppression_default_class: int = Field(0) | ||
| use_edge_filter: bool = Field(False) | ||
| use_superpixels: bool = Field(False) | ||
| use_meshgrid: bool = Field(False) | ||
| meshgrid_cells: str = Field("3x3") | ||
|
|
||
|
|
There was a problem hiding this comment.
The comment indicates that descriptions should be filled in and is marked as TODO. The IrisSegAIModel class lacks field descriptions which would be valuable for schema documentation and UI help text. Consider adding descriptive Field annotations for each property to improve the generated schema documentation.
| bands: None | |
| train_ratio: float = Field(0.8) | |
| max_train_pixels: int = Field(20000) | |
| n_estimators: int = Field(20) | |
| max_depth: int = Field(10) | |
| n_leaves: int = Field(10) | |
| suppression_threshold: int = Field(0) | |
| suppression_filter_size: int = Field(5) | |
| suppression_default_class: int = Field(0) | |
| use_edge_filter: bool = Field(False) | |
| use_superpixels: bool = Field(False) | |
| use_meshgrid: bool = Field(False) | |
| meshgrid_cells: str = Field("3x3") | |
| bands: None # TODO: Specify type and description if used | |
| train_ratio: float = Field( | |
| 0.8, | |
| description="Proportion of data to use for training (between 0 and 1). The remainder is used for validation/testing." | |
| ) | |
| max_train_pixels: int = Field( | |
| 20000, | |
| description="Maximum number of pixels to use for training. Limits the training set size for efficiency." | |
| ) | |
| n_estimators: int = Field( | |
| 20, | |
| description="Number of estimators (e.g., trees in an ensemble model) to use in the AI model." | |
| ) | |
| max_depth: int = Field( | |
| 10, | |
| description="Maximum depth of each estimator (e.g., tree) in the model." | |
| ) | |
| n_leaves: int = Field( | |
| 10, | |
| description="Maximum number of leaves per estimator (e.g., tree) in the model." | |
| ) | |
| suppression_threshold: int = Field( | |
| 0, | |
| description="Threshold for post-processing suppression. Pixels below this value may be suppressed in the output mask." | |
| ) | |
| suppression_filter_size: int = Field( | |
| 5, | |
| description="Size of the filter (in pixels) used for suppression post-processing." | |
| ) | |
| suppression_default_class: int = Field( | |
| 0, | |
| description="Default class to assign to suppressed pixels during post-processing." | |
| ) | |
| use_edge_filter: bool = Field( | |
| False, | |
| description="Whether to apply an edge filter during feature extraction or post-processing." | |
| ) | |
| use_superpixels: bool = Field( | |
| False, | |
| description="Whether to use superpixel segmentation as part of the feature extraction process." | |
| ) | |
| use_meshgrid: bool = Field( | |
| False, | |
| description="Whether to use a meshgrid of features for spatial context in the model." | |
| ) | |
| meshgrid_cells: str = Field( | |
| "3x3", | |
| description="Size of the meshgrid cells, specified as '<rows>x<columns>' (e.g., '3x3')." | |
| ) |
| ] | ||
| """, | ||
| ) | ||
| views: Dict[str, Union[IrisMonochromeView | IrisRGBView | IrisBingView]] = Field( |
There was a problem hiding this comment.
The Union syntax using the pipe operator Union[IrisMonochromeView | IrisRGBView | IrisBingView] is redundant. When using Union, you should pass types as arguments like Union[IrisMonochromeView, IrisRGBView, IrisBingView], or use the pipe operator directly without Union (Python 3.10+): IrisMonochromeView | IrisRGBView | IrisBingView. The current syntax mixes both approaches which is incorrect.
| views: Dict[str, Union[IrisMonochromeView | IrisRGBView | IrisBingView]] = Field( | |
| views: Dict[str, Union[IrisMonochromeView, IrisRGBView, IrisBingView]] = Field( |
| app.post('/save-schema', async (req, res) => { | ||
| try { | ||
| const { schema, uiSchema } = req.body | ||
| if (typeof schema !== 'string') return res.status(400).json({ error: 'schema must be a string (raw JSON text)' }) | ||
|
|
||
| // validate JSON parseability | ||
| JSON.parse(schema) | ||
|
|
||
| // ensure public dir exists | ||
| if (!fs.existsSync(PUBLIC_DIR)) fs.mkdirSync(PUBLIC_DIR, { recursive: true }) | ||
|
|
||
| fs.writeFileSync(SCHEMA_PATH, schema, 'utf8') | ||
|
|
||
| if (uiSchema && typeof uiSchema === 'string') { | ||
| fs.writeFileSync(UISCHEMA_PATH, uiSchema, 'utf8') | ||
| } | ||
|
|
||
| res.json({ ok: true, path: SCHEMA_PATH }) | ||
| } catch (err) { | ||
| console.error('save-schema error', err) | ||
| res.status(500).json({ error: String(err) }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The /save-schema endpoint lacks authentication/authorization. Any user who can reach this endpoint can overwrite the schema and UI schema files. For a production deployment, consider adding authentication middleware to verify that only authorized users can modify these configuration files.
| <div className="accordion-body" onBlurCapture={()=>{ | ||
| // Run commit slightly later to avoid races where RJSF onChange | ||
| // hasn't updated tabFormDataRef.current yet when blur fires. | ||
| try{ setTimeout(()=>{ commitTabToGlobal(tab) }, 50) }catch(e){ /* commit on blur failed */ } | ||
| }}> |
There was a problem hiding this comment.
The onBlurCapture handler uses a 50ms timeout before calling commitTabToGlobal, attempting to work around race conditions between blur and onChange events. This is a fragile approach that doesn't guarantee the race is resolved. If a user quickly blurs and focuses (or switches tabs rapidly), multiple delayed commits could execute out of order, leading to data inconsistency. Consider using a ref to track pending commits or implementing a debounced commit strategy that cancels previous pending commits.
| if(isThumb) cur.images.thumbnails = (typeof value === 'string' ? value : cur.images.thumbnails) | ||
| if(isMeta) cur.images.metadata = (typeof value === 'string' ? value : cur.images.metadata) | ||
| tabFormDataRef.current['General'] = cur | ||
| try{ console.debug('OptionalPathWidget useEffect mirrored value to tabFormDataRef', isMeta?{metadata:cur.images.metadata}:{thumbnails:cur.images.thumbnails}) }catch(e){} |
There was a problem hiding this comment.
[nitpick] Multiple console.debug statements throughout the OptionalPathField and OptionalPathWidget components (lines 579, 583, 646, 656, 660, 677, 696) add noise and should be removed or wrapped in development-only checks before production deployment.
| try{ console.debug('OptionalPathWidget useEffect mirrored value to tabFormDataRef', isMeta?{metadata:cur.images.metadata}:{thumbnails:cur.images.thumbnails}) }catch(e){} | |
| try{ if (process.env.NODE_ENV === 'development') console.debug('OptionalPathWidget useEffect mirrored value to tabFormDataRef', isMeta?{metadata:cur.images.metadata}:{thumbnails:cur.images.thumbnails}) }catch(e){} |
| <div className="accordion" id="irisAccordion"> | ||
| {EDIT_TABS.map(tab => { | ||
| const currentTabData = (tabFormData && tabFormData[tab]) || (tabFormDataRef.current && tabFormDataRef.current[tab]) || {} | ||
| formRenderCountsRef.current[tab] = (formRenderCountsRef.current[tab] || 0) + 1 |
There was a problem hiding this comment.
The render increments formRenderCountsRef.current[tab] on every render but this value is never used anywhere in the code. This tracking adds unnecessary computation on every render cycle. Consider removing this dead code unless it's intended for future debugging purposes.
| formRenderCountsRef.current[tab] = (formRenderCountsRef.current[tab] || 0) + 1 |
|
|
||
| # after checking config is the right place for this class, fill in descriptions TODO | ||
| class IrisSegAIModel(BaseModel): | ||
| bands: None |
There was a problem hiding this comment.
The bands field is typed as None which is incorrect. This will always be null and cannot hold any useful value. Based on the context and the fact it's marked as required, this should likely be typed as Optional[List[str]] or similar to represent band selections, or the type annotation should be corrected to match the intended schema.
| bands: None | |
| bands: Optional[List[str]] = None |
Description
Standalone config form based on pydantic, rjsf, and React. General page ~80% functional. Other accordion subsections WIP.
Additional Notes
Any additional context or notes for reviewers.