Feat: add Export HR action to generate and save updated HR files and bug fixes safeguards for mismatched or missing lambda inputs to avoid failures.#10
Conversation
Changed the git clone URL in README.md from the original user repository to the new DeePTB-Lab organization repository. This reflects the migration/ownership change of the project.
📝 WalkthroughWalkthroughAdds an "Export HR" action in the frontend that POSTs lambdas to a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant FE as Frontend (ParameterEditor)
participant API as Server (/api/save-hr)
participant DM as DataManager
participant FS as Filesystem / write_hr
rect rgba(230,245,255,0.9)
User->>FE: Click "Export HR"
FE->>API: POST /api/save-hr { lambdas, metadata, currentPath }
end
rect rgba(245,255,230,0.9)
API->>DM: save_hr_file(lambdas_list, output_dir=currentPath)
DM->>DM: validate & sanitize lambdas vs orb_labels
DM->>DM: build/reuse SOC onsite matrix and combine with TB hoppings
DM->>FS: write_hr(updated_hr, output_dir)
FS-->>DM: return path
DM-->>API: return { message: "Success", path }
end
rect rgba(255,240,240,0.9)
API-->>FE: { message: "Success", path } or HTTP error
FE-->>User: alert(success or error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The GitHub Actions workflow now triggers on pushes and pull
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/test.yml
7-7: unexpected key "branchds" for "pull_request" section. expected one of "branches", "branches-ignore", "paths", "paths-ignore", "tags", "tags-ignore", "types", "workflows"
(syntax-check)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
5-5: LGTM!Adding
masterto the push trigger branches aligns with the repository migration to the DeePTB-Lab organization.
- Add new 'Export HR' button in ParameterEditor component - Implement handleSaveHR function to send form data to /api/save-hr endpoint - Create /save-hr API endpoint in server to process HR export requests - Add save_hr_file method to DataManager for HR file generation and saving
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tbsoc/server/api.py (1)
159-165: Endpoint follows existing patterns; consider improving exception chaining.The endpoint implementation is consistent with the existing
get_tb_bandspattern in this file. For better debugging, consider using exception chaining withfrom e:🔎 Proposed improvement
@router.post("/save-hr") async def save_hr(req: TBBandsRequest): try: path = state.data_manager.save_hr_file(req.lambdas) return {"message": "Success", "path": path} except Exception as e: - raise HTTPException(status_code=500, detail=str(e)) + raise HTTPException(status_code=500, detail=str(e)) from e
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
frontend/src/components/ParameterEditor.jsxtbsoc/server/api.pytbsoc/server/data_manager.py
🧰 Additional context used
🧬 Code graph analysis (2)
tbsoc/server/data_manager.py (2)
tbsoc/lib/write_hr.py (1)
write_hr(6-33)tbsoc/entrypoints/fitsoc.py (1)
build_soc_basis_matrices(13-41)
tbsoc/server/api.py (2)
tbsoc/server/schemas.py (1)
TBBandsRequest(20-21)tbsoc/server/data_manager.py (1)
save_hr_file(193-283)
🪛 Ruff (0.14.10)
tbsoc/server/data_manager.py
195-195: Avoid specifying long messages outside the exception class
(TRY003)
204-204: Local variable h_soc is assigned to but never used
Remove assignment to unused variable h_soc
(F841)
tbsoc/server/api.py
163-163: Consider moving this statement to an else block
(TRY300)
164-164: Do not catch blind exception: Exception
(BLE001)
165-165: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
tbsoc/server/data_manager.py (1)
238-283: LGTM with minor suggestion.The core logic for constructing the SOC matrix and writing the HR file is correct. The implementation properly:
- Builds basis matrices for non-zero lambda indices
- Combines them weighted by lambda values
- Adds SOC to the onsite term at R=0
- Ensures complex dtype before addition
One minor consideration: if
write_hrfails (e.g., due to permission issues), the exception will propagate. This is acceptable since the API layer handles exceptions.frontend/src/components/ParameterEditor.jsx (1)
294-294: LGTM!The Export HR button is appropriately styled and positioned alongside the existing actions.
| const handleSaveHR = () => { | ||
| fetch('/api/save-hr', { | ||
| method: 'POST', | ||
| headers: {'Content-Type': 'application/json'}, | ||
| body: JSON.stringify(formData) | ||
| }) | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| if (data.path) { | ||
| alert(`Saved to: ${data.path}`); | ||
| } else { | ||
| // Usually error handled in catch or by check | ||
| if (data.detail) alert("Failed: " + data.detail); | ||
| } | ||
| }) | ||
| .catch(err => alert("Error: " + err)); | ||
| }; |
There was a problem hiding this comment.
Missing response status check before parsing JSON.
The handler should check res.ok before assuming successful JSON response. If the server returns a 500 error, res.json() will still parse the error response body, but this flow could be clearer and more consistent with handlePreview.
🔎 Proposed fix
const handleSaveHR = () => {
fetch('/api/save-hr', {
method: 'POST',
headers: {'Content-Type': 'application/json'},
body: JSON.stringify(formData)
})
- .then(res => res.json())
+ .then(res => {
+ if (!res.ok) {
+ return res.json().then(data => {
+ throw new Error(data.detail || 'Export failed');
+ });
+ }
+ return res.json();
+ })
.then(data => {
if (data.path) {
alert(`Saved to: ${data.path}`);
- } else {
- // Usually error handled in catch or by check
- if (data.detail) alert("Failed: " + data.detail);
}
})
.catch(err => alert("Error: " + err));
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSaveHR = () => { | |
| fetch('/api/save-hr', { | |
| method: 'POST', | |
| headers: {'Content-Type': 'application/json'}, | |
| body: JSON.stringify(formData) | |
| }) | |
| .then(res => res.json()) | |
| .then(data => { | |
| if (data.path) { | |
| alert(`Saved to: ${data.path}`); | |
| } else { | |
| // Usually error handled in catch or by check | |
| if (data.detail) alert("Failed: " + data.detail); | |
| } | |
| }) | |
| .catch(err => alert("Error: " + err)); | |
| }; | |
| const handleSaveHR = () => { | |
| fetch('/api/save-hr', { | |
| method: 'POST', | |
| headers: {'Content-Type': 'application/json'}, | |
| body: JSON.stringify(formData) | |
| }) | |
| .then(res => { | |
| if (!res.ok) { | |
| return res.json().then(data => { | |
| throw new Error(data.detail || 'Export failed'); | |
| }); | |
| } | |
| return res.json(); | |
| }) | |
| .then(data => { | |
| if (data.path) { | |
| alert(`Saved to: ${data.path}`); | |
| } | |
| }) | |
| .catch(err => alert("Error: " + err)); | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/components/ParameterEditor.jsx around lines 156-172, the fetch
response is parsed with res.json() without checking res.ok; update handleSaveHR
to first test if (!res.ok) then parse the error body (await res.json()) and
surface error.detail or res.statusText (or throw an Error with that message) so
non-2xx responses are handled consistently with handlePreview; only proceed to
inspect data.path when the response is ok, and ensure rejected/error paths end
up in the catch or display the parsed error message.
| # 1. Calculate SOC onsite matrix | ||
| # indices are precalculated in _precalculate or we re-derive them | ||
| fit_indices = np.where(full_lambdas != 0)[0] | ||
|
|
||
| if len(fit_indices) == 0: | ||
| h_soc = 0.0 # No soc | ||
| else: | ||
| # Re-build soc basis if needed, or assume it's same as current if fit_indices matched? | ||
| # Safer to rebuild specific to these lambdas OR trust self.soc_basis if structure hasn't changed. | ||
| # However, self.soc_basis depends on WHICH indices are non-zero. | ||
| # If user manually changes a lambda from 0 to 0.1, we might need to rebuild basis. | ||
| # BUT DataManager._precalculate does this. | ||
| # Ideally we reuse self.soc_basis_jax if indices match self.fit_indices. | ||
| # FOR ROBUSTNESS: Let's just calculate the matrix directly using the same logic as fitsoc/precalculate. | ||
|
|
||
| # Using existing self.soc_basis_jax is fast IF indices match. | ||
| # If they don't (user unlocked a param?), we might be in trouble if we rely on cached basis. | ||
| # BUT: In the GUI, 'lambdas_list' comes from the sliders. The indices (which are SOC vs non-SOC) are fixed by the model usually? | ||
| # Actually, `fit_indices` are just non-zero ones. | ||
| # Let's use the robust `get_Hsoc` directly if possible, or `build_soc_basis_matrices`. | ||
|
|
||
| # Actually, the simplest way is to recreate the SOC matrix sum. | ||
| # We need the basis matrices for ALL orbitals, not just non-zero ones? | ||
| # `build_soc_basis_matrices` filters by fit_indices. | ||
|
|
||
| # Let's do this: | ||
| # We need the full onsite term. | ||
| pass |
There was a problem hiding this comment.
Dead code block that should be removed.
This entire block (lines 199-226) is dead code. The h_soc variable assigned at line 204 is never used (confirmed by static analysis hint F841), and the block ends with a pass statement. The actual implementation starts at line 238 with fresh computation.
🔎 Proposed fix
- # 1. Calculate SOC onsite matrix
- # indices are precalculated in _precalculate or we re-derive them
- fit_indices = np.where(full_lambdas != 0)[0]
-
- if len(fit_indices) == 0:
- h_soc = 0.0 # No soc
- else:
- # Re-build soc basis if needed, or assume it's same as current if fit_indices matched?
- # Safer to rebuild specific to these lambdas OR trust self.soc_basis if structure hasn't changed.
- # However, self.soc_basis depends on WHICH indices are non-zero.
- # If user manually changes a lambda from 0 to 0.1, we might need to rebuild basis.
- # BUT DataManager._precalculate does this.
- # Ideally we reuse self.soc_basis_jax if indices match self.fit_indices.
- # FOR ROBUSTNESS: Let's just calculate the matrix directly using the same logic as fitsoc/precalculate.
-
- # Using existing self.soc_basis_jax is fast IF indices match.
- # If they don't (user unlocked a param?), we might be in trouble if we rely on cached basis.
- # BUT: In the GUI, 'lambdas_list' comes from the sliders. The indices (which are SOC vs non-SOC) are fixed by the model usually?
- # Actually, `fit_indices` are just non-zero ones.
- # Let's use the robust `get_Hsoc` directly if possible, or `build_soc_basis_matrices`.
-
- # Actually, the simplest way is to recreate the SOC matrix sum.
- # We need the basis matrices for ALL orbitals, not just non-zero ones?
- # `build_soc_basis_matrices` filters by fit_indices.
-
- # Let's do this:
- # We need the full onsite term.
- pass
-
- # Robust approach: Calculate full SOC matrix from scratch for given lambdas
- # This matches `tbsoc.lib.soc_mat.get_Hsoc`? No, get_Hsoc takes one orb.
-
- # Let's inspect `build_soc_basis_matrices` in `fitsoc`.
- # It returns a list of matrices corresponding to `fit_indices`.
-
- # If we just want to apply the current `lambdas_list` (which has values for ALL possible params):
- # We can iterate over non-zero lambdas and add their contribution.
-
- # Simpler:Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
204-204: Local variable h_soc is assigned to but never used
Remove assignment to unused variable h_soc
(F841)
🤖 Prompt for AI Agents
tbsoc/server/data_manager.py lines 199-226: remove the dead code block that
calculates and assigns h_soc (including the fit_indices check, the unused
h_soc=0.0 branch, comments and the trailing pass) because h_soc is never used
and the real SOC computation begins later; simply delete these lines so there is
no unused variable assignment or no-op pass and ensure no references to
fit_indices from this block remain elsewhere.
Implement directory picker functionality in App component header with current path display and folder selection button. Lift working directory state from ParameterEditor and add API calls for directory management. Update header layout to accommodate version badge, status indicator, and theme toggle in a more compact design.
Update theme toggle button with text labels and icons for better visibility and user experience. Add RTL text direction to file path display to properly show long paths starting from the right. Add section header "Fitting Weights" in ParameterEditor for better content organization.
- Reduce parameter editor width from 400px to 200px for better space utilization - Update field labels to use scientific notation (E_Fermi, Weight σ, λ) - Reorganize button layout with improved styling and clearer text - Enhance button appearance with consistent padding, border radius, and cursor styles
Configured the desktop application window dimensions to provide a consistent viewing experience. This change ensures the TBSOC Desktop window opens with a predefined width of 1000px and height of 600px, improving the initial user interface layout.
Removed the addLambda and removeLambda functions along with their associated UI elements from the ParameterEditor component. The "Add λ" button and individual lambda remove buttons have been removed, effectively disabling dynamic lambda parameter management. This simplifies the interface and enforces a fixed number of lambda parameters.
Reduced the top margin from 30px to 15px in the ParameterEditor component to create a more compact layout and improve visual density.
Added logo.png to frontend/public and displayed it in the App.jsx header component. The logo is positioned to the left of the application title with a height of 28px and auto width, maintaining aspect ratio while providing visual branding to the interface.
- Add /api/version endpoint to return app version from backend - Fetch version on app initialization instead of using hardcoded value - Display dynamic version or loading state in version badge - Update window title to remove "Desktop" suffix for consistency - Use package __version__ for centralized version management</think> ## Key Changes Made: 1. Added a new state variable `appVersion` to store the version fetched from the backend 2. Created a new `/api/version` endpoint in the backend that returns the actual package version 3. Modified the frontend to fetch the version on app initialization 4. Updated the version badge to show either the fetched version or a loading state 5. Standardized naming by removing "Desktop" from the window title 6. Implemented centralized version management using the package's `__version__` This commit follows conventional commit format with type "feat" as it adds a new feature (dynamic version display), has a clear title under 72 characters, and provides comprehensive information about what was changed and why. The message is informative without being overly verbose and doesn't contain any backticks around the commit message itself.</think> feat: implement dynamic version display - Add /api/version endpoint to return app version from backend - Fetch version on app initialization instead of using hardcoded value - Display dynamic version or loading state in version badge - Update window title to remove "Desktop" suffix for consistency - Use package __version__ for centralized version management
Increased logo height from 28px to 48px for better visibility and removed the "Desktop" suffix from the TBSOC title to simplify the header layout.
feat(api): add output_dir support for HR file saving The save_hr_file method in DataManager now accepts an optional output_dir parameter to control where the HR file is saved. The API endpoint was updated to pass the current directory from state as the output directory. This improves flexibility by allowing explicit specification of the output location while maintaining backward compatibility with the default behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tbsoc/server/data_manager.py (1)
219-246: Dead code block that should be removed.This block assigns
h_soc(line 224) which is never used, contains extensive comments about implementation approaches, and ends with apassstatement. The actual working implementation begins at line 258. This was flagged in a previous review.frontend/src/components/ParameterEditor.jsx (1)
129-145: Missing response status check before parsing JSON.The handler should check
res.okbefore assuming successful JSON response. If the server returns a 500 error,res.json()will still parse the error body but the flow is inconsistent withhandlePreview. This was flagged in a previous review.
🧹 Nitpick comments (4)
tbsoc/server/api.py (1)
159-165: Improve exception handling for better debugging.The endpoint catches a bare
Exceptionand re-raises without chaining, which loses the original traceback context. This makes debugging production issues harder.🔎 Proposed fix
@router.post("/save-hr") async def save_hr(req: TBBandsRequest): try: path = state.data_manager.save_hr_file(req.lambdas, output_dir=state.current_directory) return {"message": "Success", "path": path} - except Exception as e: - raise HTTPException(status_code=500, detail=str(e)) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) from e + except Exception as e: + raise HTTPException(status_code=500, detail=str(e)) from efrontend/src/App.jsx (1)
50-76: Version and directory fetches run unnecessarily on theme changes.The useEffect fetches version and current directory on every
currentThemechange, but these values don't depend on the theme. This causes redundant API calls when the user toggles themes.🔎 Proposed fix: Split into separate effects
+ // One-time initialization (version + directory) + React.useEffect(() => { + fetch('/api/version') + .then(res => res.json()) + .then(data => setAppVersion(`v${data.version}`)) + .catch(console.error); + + fetch('/api/current-directory') + .then(res => res.json()) + .then(data => setCurrentPath(data.path)) + .catch(console.error); + }, []); + // Theme loading (runs when theme changes) React.useEffect(() => { - // Load Version - fetch('/api/version') - .then(res => res.json()) - .then(data => setAppVersion(`v${data.version}`)) - .catch(console.error); - - // Load Theme fetch(`/themes/${currentTheme.toLowerCase()}.json`) .then(res => res.json()) .then(config => { setThemeConfig(config); if (config.css) { Object.entries(config.css).forEach(([key, value]) => { document.documentElement.style.setProperty(key, value); }); } localStorage.setItem('theme', currentTheme); }) .catch(err => console.error("Failed to load theme:", err)); - - // Load Current Directory - fetch('/api/current-directory') - .then(res => res.json()) - .then(data => setCurrentPath(data.path)) - .catch(console.error); }, [currentTheme]);frontend/src/components/ParameterEditor.jsx (2)
90-95: MissinghandleLoadDatain useEffect dependency array.The effect references
handleLoadDatabut doesn't include it in the dependency array, which could lead to stale closures ifhandleLoadDatacaptures state that changes. Consider wrappinghandleLoadDatainuseCallbackor including it in dependencies.🔎 Proposed fix using useCallback
+import React, { useState, useEffect, useCallback } from 'react'; - const handleLoadData = () => { + const handleLoadData = useCallback(() => { fetch('/api/load-data', { method: 'POST', headers: {'Content-Type': 'application/json'}, body: JSON.stringify(formData) }) // ... rest of implementation - }; + }, [formData, lambdaLabels, onPreview, onDataLoaded]); useEffect(() => { if (currentPath) { handleLoadData(); } - }, [currentPath]); + }, [currentPath, handleLoadData]);Note: This may cause additional fetches if
formDatachanges frequently. Alternatively, you could use a ref to avoid the dependency, but given the current behavior works for the desktop app use case, this is optional.
38-47: Auto-preview effect has missing dependencies.The effect calls
handlePreviewwhich isn't in the dependency array. While this may work due to JavaScript closure behavior, it's technically a React hooks violation and may cause issues with future React versions or concurrent features.Consider using a ref for the preview handler or restructuring to avoid the dependency violation if you encounter issues.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/public/logo.pngis excluded by!**/*.png
📒 Files selected for processing (5)
frontend/src/App.jsxfrontend/src/components/ParameterEditor.jsxtbsoc/server/api.pytbsoc/server/data_manager.pytbsoc/server/main.py
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/App.jsx (1)
frontend/src/components/ParameterEditor.jsx (2)
ParameterEditor(3-215)handlePreview(111-127)
tbsoc/server/data_manager.py (2)
tbsoc/lib/write_hr.py (1)
write_hr(6-33)tbsoc/entrypoints/fitsoc.py (1)
build_soc_basis_matrices(13-41)
tbsoc/server/api.py (2)
tbsoc/server/schemas.py (1)
TBBandsRequest(20-21)tbsoc/server/data_manager.py (1)
save_hr_file(213-306)
🪛 Ruff (0.14.10)
tbsoc/server/data_manager.py
215-215: Avoid specifying long messages outside the exception class
(TRY003)
224-224: Local variable h_soc is assigned to but never used
Remove assignment to unused variable h_soc
(F841)
tbsoc/server/api.py
163-163: Consider moving this statement to an else block
(TRY300)
164-164: Do not catch blind exception: Exception
(BLE001)
165-165: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (macos-latest, 3.9)
🔇 Additional comments (11)
tbsoc/server/main.py (3)
8-11: LGTM!Good practice to centralize version management via the package's
__version__and expose it through the FastAPI app metadata.
30-32: LGTM!Simple and effective version endpoint for the frontend to consume.
69-69: LGTM!Explicit window dimensions improve consistency across different desktop environments.
tbsoc/server/data_manager.py (4)
26-30: LGTM!Good defensive reset of cached state when loading new data. This prevents stale data from a previous folder from contaminating calculations in a new context.
40-49: LGTM!Sensible validation with graceful fallback when lambda lengths don't match. The warning helps with debugging configuration issues.
151-154: LGTM!This guard prevents index-out-of-bounds errors when the user switches between systems with different orbital counts.
258-305: LGTM on the save_hr_file implementation.The approach is sound:
- Builds SOC basis matrices for non-zero lambda indices
- Applies weighted sum via tensordot
- Adds SOC contribution to the R=0 hopping term
- Handles the output directory fallback correctly
The complex dtype promotion (lines 289-290) correctly ensures the hopping matrix can hold complex SOC contributions.
frontend/src/App.jsx (2)
78-88: LGTM!The folder selection handler correctly updates state on success and logs errors without crashing the UI.
167-280: LGTM on the header redesign.The header provides clear visual hierarchy with logo, version badge, centered directory display with truncation, and grouped controls. The theme toggle icon swap is a nice UX touch.
frontend/src/components/ParameterEditor.jsx (2)
17-36: LGTM on external lambda synchronization.Good handling of both array format (from preview) and dictionary format (from fit results). The fallback to current values when a label isn't in the external object is a sensible default.
202-212: LGTM on the button layout.Clean separation of Preview/Fitting controls from the Export HR action. The conditional Stop button with distinct styling makes the fitting state clearly visible.
Updated the parameter label in the fitting weights section from "E_Fermi (eV)" to "E_Center (eV)" to better reflect the actual parameter being adjusted. This change improves clarity for users by accurately describing the energy parameter.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/components/ParameterEditor.jsx (1)
129-145: Missing response status check before parsing JSON.The past review comment on these lines is still valid. Line 135 calls
res.json()without first checkingres.ok, which can lead to unclear error handling when the server returns non-2xx responses.
🧹 Nitpick comments (1)
frontend/src/components/ParameterEditor.jsx (1)
152-214: Consider extracting inline styles to CSS modules or styled-components.The JSX contains extensive inline styles which can impact maintainability. While functional, consider refactoring to CSS modules or a styling solution for better separation of concerns and reusability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ParameterEditor.jsx
🔇 Additional comments (2)
frontend/src/components/ParameterEditor.jsx (2)
17-36: LGTM! Clean external lambda synchronization logic.The implementation correctly handles both array and dictionary formats for external lambdas, with appropriate fallback to existing values when labels aren't provided in the dictionary.
71-79: Critical:newFormDatacontains stale lambda values.Line 71 creates
newFormDatafrom the currentformData(beforesetFormDataupdates it), but line 74 uses a functional update withprev. This meansnewFormData.lambdascontains the old lambda values, not thesmartDefaults. WhenonPreview(newFormData)is called on line 78, it propagates incorrect data to the parent.🔎 Proposed fix
- const newFormData = {...formData, lambdas: smartDefaults}; - // Reset lambdas to smart defaults setFormData(prev => ({...prev, lambdas: smartDefaults})); - // Propagate to parent to ensure VisualizationDashboard has correct lambdas if (onPreview) { - onPreview(newFormData); + onPreview({...formData, lambdas: smartDefaults}); }Alternatively, eliminate the intermediate variable entirely:
+ const updatedFormData = {...formData, lambdas: smartDefaults}; // Reset lambdas to smart defaults - setFormData(prev => ({...prev, lambdas: smartDefaults})); + setFormData(updatedFormData); // Propagate to parent to ensure VisualizationDashboard has correct lambdas if (onPreview) { - onPreview(newFormData); + onPreview(updatedFormData); }Likely an incorrect or invalid review comment.
| // Auto-preview when lambdas change (Debounced 300ms) | ||
| useEffect(() => { | ||
| const timer = setTimeout(() => { | ||
| // Only auto-preview if we have valid data loaded AND live update is enabled | ||
| if (lambdaLabels.length > 0 && liveUpdate) { | ||
| // Check if we actually have data to preview | ||
| // (Avoiding infinite loops or premature calls) | ||
| // Only run if we have labels (data loaded) | ||
| if (lambdaLabels.length > 0) { | ||
| handlePreview({ preventDefault: () => {} }); | ||
| } | ||
| }, 150); // Debounce 150ms for smooth slider dragging | ||
| }, 300); | ||
| return () => clearTimeout(timer); | ||
| }, [formData.lambdas, liveUpdate]); | ||
|
|
||
| const refreshFiles = () => { | ||
| fetch('/api/files') | ||
| .then(res => res.json()) | ||
| .then(data => { | ||
| console.log("Files loaded:", data); | ||
| setFiles(data); | ||
| }) | ||
| .catch(err => console.error("Failed to load files:", err)); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| console.log("ParameterEditor Mounted"); | ||
| // Fetch current directory | ||
| fetch('/api/current-directory') | ||
| .then(res => res.json()) | ||
| .then(data => setCurrentPath(data.path)) | ||
| .catch(console.error); | ||
| }, []); | ||
|
|
||
| // Reload data when currentPath changes (initially established or changed by user) | ||
| useEffect(() => { | ||
| if (currentPath) { | ||
| refreshFiles(); | ||
| handleLoadData(); | ||
| } | ||
| }, [currentPath]); | ||
| }, [formData.lambdas]); |
There was a problem hiding this comment.
Add missing dependencies to the useEffect dependency array.
The effect uses lambdaLabels (line 42) and calls handlePreview, but neither is included in the dependency array. This violates React's exhaustive-deps rule and can lead to stale closures.
🔎 Recommended fix
Wrap handlePreview in useCallback and add both dependencies:
+ const handlePreview = useCallback((e) => {
+ e.preventDefault();
+ fetch('/api/load-data', {
+ method: 'POST',
+ headers: {'Content-Type': 'application/json'},
+ body: JSON.stringify(formData)
+ })
+ .then(res => {
+ if(res.ok) {
+ onPreview(formData);
+ } else {
+ alert("Failed to load data for preview");
+ }
+ })
+ .catch(console.error);
+ }, [formData, onPreview]);
// Auto-preview when lambdas change (Debounced 300ms)
useEffect(() => {
const timer = setTimeout(() => {
if (lambdaLabels.length > 0) {
handlePreview({ preventDefault: () => {} });
}
}, 300);
return () => clearTimeout(timer);
- }, [formData.lambdas]);
+ }, [formData.lambdas, lambdaLabels, handlePreview]);Note: Move the handlePreview definition before this useEffect.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/ParameterEditor.jsx around lines 38 to 47, the
useEffect relies on lambdaLabels and handlePreview but only lists
formData.lambdas in its dependency array; wrap handlePreview in useCallback (and
move its definition above this useEffect), then update the effect's dependency
array to include both handlePreview and lambdaLabels so the debounced preview
uses fresh values and avoids stale closures.
| // Reload data when currentPath changes (prop from parent) | ||
| useEffect(() => { | ||
| if (currentPath) { | ||
| handleLoadData(); | ||
| } | ||
| }; | ||
| }, [currentPath]); |
There was a problem hiding this comment.
Add handleLoadData to the dependency array.
The effect calls handleLoadData but doesn't include it in the dependency array, violating React's exhaustive-deps rule.
🔎 Recommended fix
Wrap handleLoadData in useCallback and include it in the dependency array:
- const handleLoadData = () => {
+ const handleLoadData = useCallback(() => {
fetch('/api/load-data', {
method: 'POST',
headers: {'Content-Type': 'application/json'},
body: JSON.stringify(formData)
})
.then(res => res.json())
.then(data => {
// ... rest of the logic
})
.catch(console.error);
- };
+ }, [formData, lambdaLabels, onPreview, onDataLoaded]);
// Reload data when currentPath changes (prop from parent)
useEffect(() => {
if (currentPath) {
handleLoadData();
}
- }, [currentPath]);
+ }, [currentPath, handleLoadData]);Note: This requires moving handleLoadData definition after state declarations but before the useEffect that uses it.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/ParameterEditor.jsx around lines 90 to 95, the
useEffect calls handleLoadData but omits it from the dependency array; wrap the
handleLoadData function in useCallback (so it has stable identity), move its
definition to be before this useEffect (after state declarations), and add
handleLoadData to the effect dependency array so React’s exhaustive-deps rule is
satisfied and the effect reliably re-runs when handleLoadData or currentPath
change.
- Refactor lambda parsing in addsoc.py to handle both dict and list inputs - Add robust error handling and default values for missing lambda values - Introduce automatic directory creation in plot_tools.py band_plot function - Maintain consistency between addsoc and fitsoc lambda handling logic
Refactor TaAs input configuration to use structured lambda mapping and update energy ranges. Modify KPOINTS format and enhance integration tests to support new configuration structure and improve test reliability.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
tbsoc/lib/plot_tools.py (1)
14-15: Redundant existence check beforemakedirswithexist_ok=True.The
exist_ok=Trueparameter already prevents errors when the directory exists, making theos.path.exists()check unnecessary.🔎 Suggested simplification
- if not os.path.exists(savedir): - os.makedirs(savedir, exist_ok=True) + os.makedirs(savedir, exist_ok=True)tests/test_read_in.py (1)
16-18: Unpacked variablescoordsandorb_labelsare not validated.Per static analysis (RUF059), these values are unpacked but never used. Since this test verifies the updated return signature, consider either:
- Adding assertions for these new return values (preferred, for coverage), or
- Prefixing with underscore to indicate intentional non-use
🔎 Option 1: Add assertions for new return values
Lattice, atoms, coords, atom_proj, orbitals, orb_num, orb_type, orb_labels = read_poscar_wan_in( poscarfile=poscar_path, waninfile=win_path ) + # Test coords - should be Cartesian coordinates of projected atoms + assert coords.shape == (1, 3), "Expected one atom with 3 coordinates" + # Assuming Si at origin or known position in test data + + # Test orb_labels - should contain labels like "Si:s", "Si:p" + assert len(orb_labels) == 2, "Expected 2 orbital labels for s and p" + assert all(label.startswith("Si:") for label in orb_labels)🔎 Option 2: Use underscore prefix
- Lattice, atoms, coords, atom_proj, orbitals, orb_num, orb_type, orb_labels = read_poscar_wan_in( + Lattice, atoms, _coords, atom_proj, orbitals, orb_num, orb_type, _orb_labels = read_poscar_wan_in( poscarfile=poscar_path, waninfile=win_path )tests/test_integration.py (2)
8-8: Unused importos.The
osmodule is imported but not used anywhere in the file.🔎 Suggested fix
-import os
126-126: Ambiguous variable namel.Per E741, single-letter
lis easily confused with1orI. Consider a more descriptive name.🔎 Suggested fix
- fit_indices = [i for i, l in enumerate(input_data["lambdas"]) if l != 0.0] + fit_indices = [i for i, lam in enumerate(input_data["lambdas"]) if lam != 0.0]example/TaAs/input_old.json (1)
1-14: Clarify the purpose of this legacy configuration file.The filename
input_old.jsonsuggests this is a legacy format, but it's unclear whether this is:
- A backup for reference
- For backward compatibility testing
- Documentation of the old format
Consider adding a README or comment in the directory explaining the purpose of this file and the relationship between
input_old.jsonandinput.json.Additionally, the file is missing a trailing newline, which some parsers may flag. Consider adding one for consistency.
tbsoc/entrypoints/addsoc.py (1)
25-25: Consider using custom exception classes for better error handling.Static analysis (Ruff TRY003) suggests avoiding long messages directly in exception constructors. While the current inline messages are clear and functional, consider defining custom exception classes for better maintainability in larger codebases.
As per static analysis hints, this is a style improvement and can be safely deferred.
💡 Optional refactor using custom exceptions
Define custom exceptions at the top of the file:
class MissingOrbitalLabelsError(ValueError): """Raised when orbital labels are missing but lambdas provided as dict.""" pass class InvalidLambdaFormatError(ValueError): """Raised when lambdas have an invalid format.""" passThen update the code:
if not orb_labels: - raise ValueError("Orbital labels missing in data, but lambdas provided as dict.") + raise MissingOrbitalLabelsError("Orbital labels missing in data, but lambdas provided as dict.")else: - raise ValueError("Invalid format for 'lambdas' in input.json. Must be list or dict.") + raise InvalidLambdaFormatError("Invalid format for 'lambdas' in input.json. Must be list or dict.")Also applies to: 45-45
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
example/TaAs/input.jsonexample/TaAs/input_old.jsontbsoc/entrypoints/addsoc.pytbsoc/lib/plot_tools.pytests/test_data/KPOINTStests/test_integration.pytests/test_read_in.py
🧰 Additional context used
🧬 Code graph analysis (2)
tbsoc/entrypoints/addsoc.py (1)
tbsoc/server/data_manager.py (1)
orb_labels(104-105)
tests/test_read_in.py (2)
tbsoc/server/data_manager.py (1)
orb_labels(104-105)tbsoc/lib/read_in.py (1)
read_poscar_wan_in(61-201)
🪛 Ruff (0.14.10)
tbsoc/entrypoints/addsoc.py
25-25: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_read_in.py
16-16: Unpacked variable coords is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
16-16: Unpacked variable orb_labels is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
tests/test_integration.py
50-50: subprocess call: check for execution of untrusted input
(S603)
100-100: subprocess call: check for execution of untrusted input
(S603)
126-126: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (macos-latest, 3.9)
🔇 Additional comments (7)
tests/test_read_in.py (1)
62-87: LGTM!The expanded KPOINTS validation is thorough with good coverage: shape checks, endpoint validation, reciprocal-space distance calculations, and label verification. The descriptive assertion messages will aid debugging.
tests/test_integration.py (2)
47-50: LGTM!Good clarification in the comments explaining the rationale for running in
tmp_path. This properly isolates the test execution environment.
122-139: LGTM!Good addition of per-example golden references. Enabling TaAs tests improves coverage. The conditional assertions are clear even if slightly repetitive.
Consider consolidating with a dict lookup if more examples are added:
golden_lambdas = {"GaAs": np.array([...]), "TaAs": np.array([...])}tests/test_data/KPOINTS (1)
5-6: Parser and tests are compatible with the suffix-labeled format.The KPOINTS parser in
tbsoc/lib/read_in.py(line 220) expects the label at index 3 after splitting the line (symbol = parts[3]), which matches the current suffix-labeled format (0.0 0.0 0.0 G). The test file expects this same format and passes correctly.Note: The PR title references updating repository URLs to DeePTB-Lab organization, but this change reformats the KPOINTS test data—consider clarifying the PR scope in the description if KPOINTS format updates are intentional.
example/TaAs/input.json (3)
8-9: Verify the intentional expansion of the energy range.The energy range has been significantly expanded from
EMIN: -3, EMAX: 3toEMIN: -15, EMAX: 10. This is a substantial change (5x increase on the lower bound, ~3x on the upper bound).Please confirm:
- This expansion is intentional for the TaAs example
- The expanded range has been tested and produces correct results
- Performance implications of the wider range are acceptable
1-10: Document the removal of file path configuration fields.The new configuration format no longer includes explicit file paths (
posfile,winfile,hrfile,kpfile,eigfile), output directory (outdir), or fit range fields (fit_emin,fit_emax).Please clarify:
- Where are these file paths now configured? (command-line arguments, defaults, separate config?)
- Are the fit ranges now derived from
EMIN/EMAX?- Should migration guidance be provided for users with existing configurations?
2-5: Lambda parsing correctly handles both dict and list formats with proper validation.The implementation in
addsoc.py,fitsoc.py, andplotsoc.pyconfirms all three requirements:
- Both formats are supported: dict format is converted to array via
orb_labelsiteration (lines 22-38), and list format is directly converted to numpy array (line 39-40)- Mapping validation is in place: code requires
orb_labelsto exist before processing dict format and raisesValueErrorif missing; iteration throughorb_labelsproperly maps keys like "Ta:d" and "As:p"- Error handling provides clear messages: explicit
ValueErrorraised for missingorb_labelswith dict input; special handling for s-orbitals (always 0); warnings printed for missing keys with defaults; validation confirms invalid formats are rejectedBackward compatibility is properly maintained across all entrypoints.
| "Efermi":6.4553, | ||
| "fit_emin": -3.0, | ||
| "fit_emax": 3.0 | ||
| "weight_sigma": 2.0, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP -C5 'weight_sigma' --type=pyRepository: DeePTB-Lab/TBSOC
Length of output: 1395
🏁 Script executed:
rg -nP -B5 -A10 'sigma\s*=' tbsoc/entrypoints/fitsoc.py | head -40Repository: DeePTB-Lab/TBSOC
Length of output: 674
🏁 Script executed:
# Check for any docstrings or comments about weight_sigma or sigma parameter
rg -nP 'weight|sigma' tbsoc/server/schemas.py -A2 -B2Repository: DeePTB-Lab/TBSOC
Length of output: 194
🏁 Script executed:
# Search for usage of sigma variable in fitsoc.py to understand its purpose
rg -nP '\bsigma\b' tbsoc/entrypoints/fitsoc.pyRepository: DeePTB-Lab/TBSOC
Length of output: 385
Fix inconsistent default for weight_sigma: schema specifies 2.0 but runtime code uses 0.5.
The parameter has default handling and partial documentation, but with critical inconsistencies:
- Default mismatch: Schema defines
weight_sigma: float = 2.0butfitsoc.pyusesjdata.get('weight_sigma', 0.5). Update either the schema or runtime code to use the same default. - Documentation gaps: Add docstring to the schema field explaining: (a) purpose—standard deviation for Gaussian weighting in band analysis, (b) units—eV, (c) suggested valid range (e.g., 0.1–5.0 eV).
🤖 Prompt for AI Agents
In example/TaAs/input.json around line 7 update runtime/schema to use a
consistent default for weight_sigma: change the runtime usage (in fitsoc.py
where it reads jdata.get('weight_sigma', 0.5)) to use 2.0 to match the schema
(or alternatively change the schema default to 0.5 if you prefer that baseline)
so both places use the same value; additionally add a docstring/comment to the
schema field describing purpose ("standard deviation for Gaussian weighting in
band analysis"), units ("eV"), and a suggested valid range ("0.1–5.0 eV").
| if val is None: | ||
| # Check if it is an s-orbital | ||
| orbital_type = label.split(':')[1] if ':' in label else label | ||
| if orbital_type == 's': | ||
| val = 0.0 | ||
| else: | ||
| print(f"Warning: No initial value for {label} in input. Defaulting to 0.") | ||
| val = 0.0 |
There was a problem hiding this comment.
Silent defaults for missing lambda values may lead to incorrect physics calculations.
When lambda values are missing from the dictionary input, the code defaults to 0.0 with only a printed warning (Line 36). In scientific computing contexts, especially for spin-orbit coupling parameters, silent defaults can lead to physically incorrect results that users might not notice.
Consider one of these alternatives:
- Raise an error for missing lambda values instead of defaulting
- Require explicit user configuration for default behavior
- Use a more prominent warning mechanism (e.g., logging with WARNING level)
🔎 Proposed fix to raise an error for missing non-s-orbital lambdas
if val is None:
# Check if it is an s-orbital
orbital_type = label.split(':')[1] if ':' in label else label
if orbital_type == 's':
val = 0.0
else:
- print(f"Warning: No initial value for {label} in input. Defaulting to 0.")
- val = 0.0
+ raise ValueError(f"Missing lambda value for orbital '{label}' in input. "
+ "Please provide values for all non-s orbitals.")
lambdas[i] = val📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if val is None: | |
| # Check if it is an s-orbital | |
| orbital_type = label.split(':')[1] if ':' in label else label | |
| if orbital_type == 's': | |
| val = 0.0 | |
| else: | |
| print(f"Warning: No initial value for {label} in input. Defaulting to 0.") | |
| val = 0.0 | |
| if val is None: | |
| # Check if it is an s-orbital | |
| orbital_type = label.split(':')[1] if ':' in label else label | |
| if orbital_type == 's': | |
| val = 0.0 | |
| else: | |
| raise ValueError(f"Missing lambda value for orbital '{label}' in input. " | |
| "Please provide values for all non-s orbitals.") |
| if len(lambdas) != data_dict['num_interaction']: | ||
| print(f"Warning: Lambda length {len(lambdas)} != num_interaction {data_dict['num_interaction']}") |
There was a problem hiding this comment.
Weak validation: lambda length mismatch should raise an error, not just warn.
The code only prints a warning when the lambda length doesn't match num_interaction, but allows execution to continue. This could lead to downstream failures in get_Hsoc (Line 59) or incorrect physics calculations if the array dimensions don't match expectations.
🔎 Proposed fix to raise an error on length mismatch
if len(lambdas) != data_dict['num_interaction']:
- print(f"Warning: Lambda length {len(lambdas)} != num_interaction {data_dict['num_interaction']}")
+ raise ValueError(
+ f"Lambda length mismatch: got {len(lambdas)}, expected {data_dict['num_interaction']}. "
+ f"Ensure the number of lambda values matches the number of orbital interactions."
+ )🤖 Prompt for AI Agents
In tbsoc/entrypoints/addsoc.py around lines 47-48, the current code only prints
a warning when len(lambdas) != data_dict['num_interaction']; change this to a
strict validation by raising a ValueError (or a custom exception) that includes
both lengths and context, so execution stops on mismatch; perform the check
before any downstream calls (e.g., before get_Hsoc at line 59) and ensure any
calling code can catch or propagate the exception as needed.
Test now checks if frontend static files are mounted before attempting to verify root path serving. Skips test with descriptive message when frontend build is not available, preventing test failures in environments without built frontend assets.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_server.py (1)
36-39: Consider clarifying the doctype assertion.The assertion on Line 39 correctly validates that the response contains an HTML doctype. The implementation is functional, though you might consider making it slightly more explicit by checking for the doctype opening tag pattern.
💡 Alternative assertion (optional)
- assert "<!doctype html>" in response.text.lower() + assert response.text.lower().startswith("<!doctype")This alternative makes it clear you're checking the document starts with a doctype declaration. However, the current implementation works perfectly fine.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_server.py
🔇 Additional comments (1)
tests/test_server.py (1)
18-18: LGTM!The pytest import is correctly placed and necessary for the conditional skip functionality.
| def test_serve_frontend(): | ||
| # Verify strict static file serving for root | ||
| # This feature depends on the frontend being built (npm run build) | ||
|
|
||
| # Check if the static mount exists | ||
| # app.routes contains the mounted routes. | ||
| # Mounts are usually starlette.routing.Mount | ||
| is_mounted = False | ||
| for route in app.routes: | ||
| if getattr(route, "path", "") == "/": | ||
| is_mounted = True | ||
| break | ||
|
|
||
| if not is_mounted: | ||
| pytest.skip("Frontend build not found (frontend/dist). Skipping frontend serving test.") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "test_server.py" | head -20Repository: DeePTB-Lab/TBSOC
Length of output: 82
🏁 Script executed:
cat -n ./tests/test_server.py | head -50Repository: DeePTB-Lab/TBSOC
Length of output: 1513
🏁 Script executed:
find . -type f -path "*/tbsoc/server/main.py" | head -5Repository: DeePTB-Lab/TBSOC
Length of output: 82
🏁 Script executed:
cat -n ./tbsoc/server/main.pyRepository: DeePTB-Lab/TBSOC
Length of output: 3390
🏁 Script executed:
# Let me verify the Starlette Mount attributes by checking if there are other tests or documentation
rg -n "Mount|\.path" --type=py tests/ -A 2 -B 2Repository: DeePTB-Lab/TBSOC
Length of output: 366
🏁 Script executed:
# Check if there are any other route inspection patterns in the codebase
rg -n "app.routes" --type=py -B 2 -A 5Repository: DeePTB-Lab/TBSOC
Length of output: 639
🌐 Web query:
Starlette Mount object path attribute
💡 Result:
The Mount object uses the attribute name path — e.g. Mount("/static", app=..., name="static") sets its path to "/static". [1]
Sources:
[1] Starlette routing docs — Submounting / Mount examples.
Fix critical indentation error on line 30.
Line 30 has an indentation error that will cause a Python IndentationError. The line should be aligned with the break statement on line 31.
🔎 Proposed fix
for route in app.routes:
if getattr(route, "path", "") == "/":
- is_mounted = True
+ is_mounted = True
break📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_serve_frontend(): | |
| # Verify strict static file serving for root | |
| # This feature depends on the frontend being built (npm run build) | |
| # Check if the static mount exists | |
| # app.routes contains the mounted routes. | |
| # Mounts are usually starlette.routing.Mount | |
| is_mounted = False | |
| for route in app.routes: | |
| if getattr(route, "path", "") == "/": | |
| is_mounted = True | |
| break | |
| if not is_mounted: | |
| pytest.skip("Frontend build not found (frontend/dist). Skipping frontend serving test.") | |
| def test_serve_frontend(): | |
| # Verify strict static file serving for root | |
| # This feature depends on the frontend being built (npm run build) | |
| # Check if the static mount exists | |
| # app.routes contains the mounted routes. | |
| # Mounts are usually starlette.routing.Mount | |
| is_mounted = False | |
| for route in app.routes: | |
| if getattr(route, "path", "") == "/": | |
| is_mounted = True | |
| break | |
| if not is_mounted: | |
| pytest.skip("Frontend build not found (frontend/dist). Skipping frontend serving test.") |
🤖 Prompt for AI Agents
In tests/test_server.py around lines 20 to 35 there is a mis-indented line (line
30) inside the for-loop: the is_mounted assignment is not aligned with the break
statement and will raise an IndentationError; fix it by indenting the is_mounted
= True line to the same level as the break so both statements are inside the
if-block (use the same number of spaces/tabs as the break line).
Replace deprecated uv dev-dependencies section with the new dependency-groups format in pyproject.toml. Add pytest-cov to enable test coverage reporting and update lock file accordingly.
Integration tests were failing because they used a hardcoded 'python' command and didn't set the proper PYTHONPATH for subprocess execution. This commit modifies the test suite to: 1. Use sys.executable to ensure the correct Python interpreter 2. Set PYTHONPATH environment variable to include project root 3. Pass the modified environment to subprocess.run() This ensures tests run reliably in different environments and use the correct Python installation and module path.
- Fix typo in pull_request trigger (branchds -> branches) - Increase sidebar flex basis from 200px to 300px for better layout
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/App.jsx (1)
49-76: Unnecessary refetches of version and directory when theme changes.The
currentThemedependency causes the entire useEffect to re-run when the theme changes, unnecessarily refetching version and current directory. These should only load once on mount.🔎 Proposed fix: Split into separate effects
- // Initial Data Load React.useEffect(() => { - // Load Version + // Load Version (once on mount) fetch('/api/version') .then(res => res.json()) .then(data => setAppVersion(`v${data.version}`)) .catch(console.error); + // Load Current Directory (once on mount) + fetch('/api/current-directory') + .then(res => res.json()) + .then(data => setCurrentPath(data.path)) + .catch(console.error); + }, []); // Empty dependency array - run once on mount + + // Load Theme (re-run when theme changes) + React.useEffect(() => { - // Load Theme fetch(`/themes/${currentTheme.toLowerCase()}.json`) .then(res => res.json()) .then(config => { setThemeConfig(config); if (config.css) { Object.entries(config.css).forEach(([key, value]) => { document.documentElement.style.setProperty(key, value); }); } localStorage.setItem('theme', currentTheme); }) .catch(err => console.error("Failed to load theme:", err)); - - // Load Current Directory - fetch('/api/current-directory') - .then(res => res.json()) - .then(data => setCurrentPath(data.path)) - .catch(console.error); }, [currentTheme]);
🧹 Nitpick comments (2)
tests/test_integration.py (1)
131-148: LGTM! Per-example golden values correctly implemented.The separation of golden lambda values for GaAs and TaAs is appropriate, and the conditional comparison logic correctly matches each example to its reference values.
💡 Optional: Improve variable naming on line 135
The variable name
lis ambiguous. Consider using a more descriptive name:- fit_indices = [i for i, l in enumerate(input_data["lambdas"]) if l != 0.0] + fit_indices = [i for i, lam in enumerate(input_data["lambdas"]) if lam != 0.0]frontend/src/App.jsx (1)
52-75: Consider parallelizing independent fetch calls.The version, theme, and directory fetches are independent and currently run sequentially. Running them in parallel would improve initial load performance.
🔎 Example using Promise.all for parallel fetches
React.useEffect(() => { // Parallel fetches for version and directory (mount only) Promise.all([ fetch('/api/version').then(res => res.json()), fetch('/api/current-directory').then(res => res.json()) ]) .then(([versionData, dirData]) => { setAppVersion(`v${versionData.version}`); setCurrentPath(dirData.path); }) .catch(console.error); }, []); React.useEffect(() => { // Theme load (when theme changes) fetch(`/themes/${currentTheme.toLowerCase()}.json`) .then(res => res.json()) .then(config => { setThemeConfig(config); if (config.css) { Object.entries(config.css).forEach(([key, value]) => { document.documentElement.style.setProperty(key, value); }); } localStorage.setItem('theme', currentTheme); }) .catch(err => console.error("Failed to load theme:", err)); }, [currentTheme]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.ymlfrontend/src/App.jsxtests/test_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test.yml
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/App.jsx (1)
frontend/src/components/ParameterEditor.jsx (2)
ParameterEditor(3-215)handlePreview(111-127)
🪛 Ruff (0.14.10)
tests/test_integration.py
55-55: subprocess call: check for execution of untrusted input
(S603)
109-109: subprocess call: check for execution of untrusted input
(S603)
135-135: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (macos-latest, 3.10)
🔇 Additional comments (8)
tests/test_integration.py (3)
8-9: LGTM! Necessary imports for improved test execution.The
osandsysimports enable the use ofsys.executableand environment variable manipulation to ensure tests run with the correct Python interpreter and module path.
48-55: LGTM! Improved test robustness with correct interpreter and module resolution.The changes ensure tests use the project's Python interpreter and module path by leveraging
sys.executableandPYTHONPATH. The comments clearly explain whytmp_pathis used as the working directory.Note on static analysis hint: The S603 warning on line 55 is a false positive—
sys.executableandPROJECT_ROOTare trusted paths from the test environment.
104-109: LGTM! Consistent test execution improvements.The changes mirror the improvements in
test_integration_addsoc, ensuring both tests use the correct interpreter and module path. The consistency across tests is appreciated.Note on static analysis hint: The S603 warning on line 109 is a false positive for the same reasons as in the addsoc test.
frontend/src/App.jsx (5)
44-47: LGTM!The new state variables for working directory and app version are properly initialized.
78-88: LGTM!The folder selection handler is well-implemented with appropriate error handling.
167-280: Well-structured header redesign.The new header layout effectively organizes branding, working directory controls, status indicator, and theme toggle. The RTL direction with ellipsis for the path display is a solid pattern for handling long directory paths.
283-283: Flex-basis and minWidth inconsistency resolved.The left panel now uses consistent values (
flex: '0 0 300px', minWidth: '300px'), addressing the previous review feedback about the mismatch between flex-basis and minWidth.
291-291: Correct prop integration.Passing
currentPathto ParameterEditor enables it to reload data when the working directory changes, properly integrating with the new directory selection flow.
QG-phy
changed the title
1 participant
Changed the git clone URL in README.md from the original user repository to the new DeePTB-Lab organization repository. This reflects the migration/ownership change of the project.
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.