Enable project sharing with Cosmograph app via new export API #42
Conversation
WalkthroughAdds global API-key management and instance registration; expands Python cosmo() API and widget traits for selection, labeling, clustering, timelines, and export; implements a Python export pipeline (parquet prep, signed upload, project create); moves Arrow IPC buffering to a cached util; rewires JS config mapping, legends, UI/layout, and adds timeline/settings/point-info components. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant P as Python API (cosmo)
participant C as cosmograph.config
participant W as Cosmograph Widget
Note over U,P: instantiate widget (api_key optional)
U->>P: cosmo(..., api_key=None)
P->>C: get_api_key()
C-->>P: global_api_key or None
alt global key present
P->>P: inject api_key into kwargs
end
P->>W: instantiate Cosmograph(kwargs)
W->>C: register_instance(self)
Note over W,C: on widget destruction
W->>C: unregister_instance(self)
sequenceDiagram
participant U as User
participant EP as export_project()
participant PQ as prepare_parquet_data
participant UP as upload_file
participant API as Cosmograph API
U->>EP: export_project(api_key, name, points, links, config)
EP->>EP: validate api_key, log
alt points or links provided
EP->>API: create_empty_project(api_key, name)
API-->>EP: project_id
EP->>PQ: prepare_parquet_data(points/links)
PQ-->>EP: parquet payload(s)
EP->>UP: upload_file(api_key, payload, project_id)
UP-->>EP: uploaded file info
end
EP->>API: create_project(api_key, name, files, cosmograph_config)
API-->>EP: project details
EP-->>U: return project id / URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
cosmograph/widget/export_project/config.py (1)
5-6: Consider making the API_BASE configurable.The API endpoint is currently hardcoded. For better flexibility, consider making this configurable through environment variables.
-API_BASE = "https://cosmograph.fly.dev/api/trpc" +import os +API_BASE = os.environ.get("COSMOGRAPH_API_BASE", "https://cosmograph.fly.dev/api/trpc")cosmograph/__init__.py (1)
27-29: Consider updating module docstring to reflect new API key functionality.The docstring contains an example but doesn't mention the API key functionality. Consider updating it to demonstrate the usage of the API key management functions.
""" Python binders to cosmograph functionality ...existing example... You can also set a global API key: >>> from cosmograph import set_api_key >>> set_api_key("your-api-key") >>> g = cosmo( ... # API key will be used automatically ... ) """cosmograph/widget/export_project/upload_file.py (1)
10-54: Add error handling for response structure and timeout configuration.The upload_file function has good error handling for request exceptions, but could be improved in a few ways:
- Line 35 assumes the response has a specific structure - consider adding additional error handling to gracefully handle unexpected response formats.
- Consider adding a timeout parameter to the requests to prevent hanging operations.
def upload_file(api_key: str, data: dict[str, Any]) -> dict[str, Any]: """Generate signed URL and upload file to Cosmograph server. Raises: ValueError: If file upload or URL generation fails Returns: Dictionary containing original data with added 'upload_url' key containing the generated upload URL """ try: response = requests.post( f"{API_BASE}/publicApi.generateSignedUploadUrl", json={ "json": { "apiKey": api_key, "fileName": data["file_name"], "contentLength": data["content_length"], "contentType": "application/parquet" }, }, + timeout=30, # Add timeout to prevent hanging operations ) response.raise_for_status() logger.info("Response: %s", json.dumps(response.json(), indent=4)) - upload_url = response.json()["result"]["data"]["json"]["url"] + response_json = response.json() + try: + upload_url = response_json["result"]["data"]["json"]["url"] + except (KeyError, TypeError) as e: + logger.error("❌ Unexpected response format: %s", response_json) + msg = f"Failed to parse upload URL from response: {e}" + raise ValueError(msg) from e except requests.RequestException as e: logger.error("❌ Failed to get upload URL: %s", e) msg = f"Failed to get upload URL: {e}" raise ValueError(msg) from e try: upload_response = requests.put( upload_url, data=data["content"], headers={"Content-Type": "application/parquet"}, + timeout=60, # Add timeout for upload (can be longer than for API calls) ) upload_response.raise_for_status() logger.info("✅ File '%s' upload completed successfully", data["file_name"]) except requests.RequestException as e: logger.error("❌ Failed to upload file '%s': %s", data["file_name"], e) msg = f"Failed to upload file: {e}" raise ValueError(msg) from e return {**data, "upload_url": upload_url}cosmograph/config.py (2)
33-41: Add return type annotation for consistency.Consider adding a return type annotation to match the style of other functions.
-def register_instance(instance): +def register_instance(instance) -> None:
43-51: Add return type annotation and fix missing newline.Missing return type annotation and newline at end of file.
-def unregister_instance(instance): +def unregister_instance(instance) -> None:And add a newline at the end of the file.
🧰 Tools
🪛 Ruff (0.8.2)
51-51: No newline at end of file
Add trailing newline
(W292)
cosmograph/widget/export_project/prepare_parquet.py (1)
8-32: Efficient parquet conversion with thorough documentation.The function properly handles in-memory conversion to Parquet format with good type annotations and detailed docstring.
Add a newline at the end of the file to fix a common linting issue.
cosmograph/widget/export_project/__init__.py (3)
14-32: Add cosmograph_config to docstring parameters.The
cosmograph_configparameter is missing from the docstring.Args: api_key: Cosmograph API key project_name: Name for the project points: DataFrame containing point data links: DataFrame containing link data + cosmograph_config: Optional configuration settings for the Cosmograph project
43-55: Consider adding exception handling for uploads.The function doesn't handle exceptions that might occur during file conversions or uploads. Consider adding try/except blocks to provide more graceful error handling.
if points is not None and not points.empty: logger.info("📦 Converting points data to parquet format...") - points_data = prepare_parquet_data(points, f"{project_name}_points.parquet") - points_data = upload_file(api_key, points_data) + try: + points_data = prepare_parquet_data(points, f"{project_name}_points.parquet") + points_data = upload_file(api_key, points_data) + except Exception as e: + logger.error("❌ Failed to prepare or upload points data: %s", e) + raise ValueError(f"Failed to process points data: {e}") from e
56-70: Consider safer dictionary access for logging.The logging statement uses multiple nested
.get()calls which could be simplified using a more robust approach.- logger.info("🎉 Project export completed successfully! Project ID: %s", - result.get("result", {}).get("data", {}).get("json", {}).get("id", "unknown")) + project_id = "unknown" + try: + project_id = result["result"]["data"]["json"]["id"] + except (KeyError, TypeError): + pass + logger.info("🎉 Project export completed successfully! Project ID: %s", project_id)js/widget.ts (1)
100-108: Consider error handling or validations for config synchronization.
updatePythonCosmographConfig()filters and syncs config keys. This is fine, but you might consider adding validation or exception handling if certain properties require additional checks before saving.cosmograph/widget/__init__.py (1)
288-298: Constructor usage of global API key.
Registering instances and autoassigning a global API key are nice additions. Ensure concurrency issues are handled if multiple widgets modify the global API key simultaneously.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
cosmograph/__init__.py(1 hunks)cosmograph/base.py(4 hunks)cosmograph/config.py(1 hunks)cosmograph/data/params_ssot.json(2 hunks)cosmograph/widget/__init__.py(5 hunks)cosmograph/widget/export_project/__init__.py(1 hunks)cosmograph/widget/export_project/config.py(1 hunks)cosmograph/widget/export_project/create_project.py(1 hunks)cosmograph/widget/export_project/prepare_parquet.py(1 hunks)cosmograph/widget/export_project/upload_file.py(1 hunks)js/config-props.json(1 hunks)js/config-props.ts(0 hunks)js/cosmograph-data.ts(0 hunks)js/helper.ts(0 hunks)js/legends.ts(2 hunks)js/widget.ts(3 hunks)package.json(1 hunks)ruff.toml(1 hunks)
💤 Files with no reviewable changes (3)
- js/helper.ts
- js/cosmograph-data.ts
- js/config-props.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
cosmograph/__init__.py (1)
cosmograph/config.py (2)
set_api_key(8-20)get_api_key(23-30)
cosmograph/base.py (1)
cosmograph/config.py (1)
get_api_key(23-30)
js/legends.ts (1)
js/helper.ts (2)
isDuckDBNumericType(16-32)isDuckDBStringType(34-38)
cosmograph/widget/__init__.py (2)
cosmograph/widget/export_project/__init__.py (1)
export_project(14-69)cosmograph/config.py (3)
register_instance(33-40)unregister_instance(43-51)get_api_key(23-30)
cosmograph/widget/export_project/__init__.py (3)
cosmograph/widget/export_project/prepare_parquet.py (1)
prepare_parquet_data(8-32)cosmograph/widget/export_project/upload_file.py (1)
upload_file(10-54)cosmograph/widget/export_project/create_project.py (1)
create_project(10-95)
🪛 Ruff (0.8.2)
cosmograph/config.py
51-51: No newline at end of file
Add trailing newline
(W292)
🪛 Biome (1.9.4)
js/legends.ts
[error] 107-107: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 113-113: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (34)
package.json (1)
13-13: Dependency version updated appropriately.The update from beta.15 to beta.18 of
@cosmograph/cosmographaligns with the new features being added to the project (API key management and point size strategy).cosmograph/widget/export_project/config.py (2)
1-4: LGTM! Good documentation and imports.The module's docstring clearly explains its purpose, and necessary imports for logging are included.
8-12: Logging configuration looks good.The logging setup is appropriate with a clear format and proper output stream configuration.
cosmograph/__init__.py (1)
28-28: Good export of API key management functions.Making the
set_api_keyandget_api_keyfunctions available at the package level improves usability for developers needing to manage API keys.ruff.toml (1)
44-44: Good addition of pycodestyle warnings to linting.Adding the "E3" and "W" rules to the linter selection will help catch more code style issues and potential problems, improving overall code quality.
js/config-props.json (1)
1-90: Well-structured configuration mapping file.This new configuration file provides a clear mapping between Python's snake_case and JavaScript's camelCase naming conventions for all Cosmograph properties. The naming is consistent and descriptive, which will facilitate communication between the Python and JavaScript components of the application.
cosmograph/config.py (3)
1-5: Well-structured global configuration module.The module provides a clean implementation of global API key management with good documentation.
8-21: LGTM: The set_api_key implementation is robust.The function properly updates both the global variable and all registered instances, ensuring consistency across the application.
23-30: LGTM: Clean implementation of get_api_key.Simple and effective implementation that returns the global API key.
cosmograph/widget/export_project/prepare_parquet.py (1)
1-6: Clean module structure with appropriate imports.Good module docstring and necessary imports for the functionality.
cosmograph/base.py (4)
16-17: Good import statement for the new API key functionality.Correctly imports the specific function needed from the config module.
149-149: LGTM: Added new point_size_strategy parameter.The new parameter follows the existing pattern with appropriate default value.
225-225: LGTM: Added new api_key parameter.The parameter is properly aligned with the global API key management system.
303-308: Well-implemented global API key integration.This change elegantly integrates the global API key while maintaining backward compatibility and allowing local overrides.
cosmograph/widget/export_project/__init__.py (2)
1-12: Well-organized imports and module structure.Clear module docstring and appropriate imports from related modules.
38-42: Good logging for process traceability.Provides clear progress logging with relevant details.
js/widget.ts (8)
4-4: Imports look consistent.
All newly introduced imports seem properly utilized in the codebase without showing any obvious issues or duplicates.Also applies to: 6-6, 10-10
112-112: Appropriate assignment of binary data to Cosmograph config.
Assigningpointsandlinksfromipc.bufferprevents type mismatches. This looks correct.Also applies to: 116-116
119-130: Event handlers for legend disabling appear fine.
These handlers correctly route tolegends.updatefor toggling legend visibility.
134-142: Property removal logic is valid.
Deleting undefined or null config keys keeps the Cosmograph config clean. No issues found.
148-148: Subscription approach works well for dynamic config updates.
The call toupdatePythonCosmographConfig()ensures Python config stays in sync, which helps maintain a single source of truth.Also applies to: 151-155
159-160: Legends updates on property changes are properly triggered.
Automatically refreshing the legend helps keep the UI consistent with new color or size properties.Also applies to: 163-164, 167-168, 171-172, 178-178
188-193: Legends auto-update on graph rebuild is correct.
Invokinglegends.updatefor each dimension upononGraphRebuiltensures the legend is always current.
198-199: Deferred config sync is good practice.
CallingupdatePythonCosmographConfig()after data preparation ensures coherence before the widget is fully rendered.cosmograph/widget/__init__.py (5)
7-10: New imports integrate well.
These traitlets and config-related imports are consistent with the rest of the file.
121-121: New traitlets expand configuration options.
point_size_strategyandshow_labels_forare seamlessly integrated with.tag(sync=True)for JS-Python synchronization.Also applies to: 150-150
182-182:cosmograph_configandapi_keytrait definitions.
Defining these as a dictionary and Unicode is appropriate, but consider default validation if usage requires specific formats or types.Also applies to: 184-184
285-287: Project export method is straightforward.
export_project_by_namepulls from local data and config, which fosters easy reuse.
299-302: Destructor ensures clean instance unregistration.
Unregistering the instance helps avoid dangling references.js/legends.ts (5)
10-10: Import usage for DuckDB type checks.
Bringing inisDuckDBStringTypeandisDuckDBNumericTypeis congruent with verifying column types for color strategy logic.
12-29: Enums for color and size strategies are well-structured.
These enumerations clarify the approach to color/size handling, improving maintainability and readability.
31-43:getPointColorLegendTypefunction is clear.
This logic systematically differentiates numeric vs. string columns to select the appropriate color type.
96-98: Updated method signature for legend updates.
RenamingupdateLegendtoupdateis succinct and clarifies the method's purpose.
126-130: Visibility update method is straightforward.
The_updateVisibilityfunction is well-defined and keeps the legend presentation logic separate.
These changes were made with the assistance of Code Rabbit.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/legends.ts (1)
96-129: Case-block scoping and feature parity
Using curly braces in each switch case properly contains variable declarations and addresses past review concerns. Consider whether link color should also differentiate string vs. numeric data in a manner similar to point color.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cosmograph/config.py(1 hunks)cosmograph/data/params_ssot.json(2 hunks)cosmograph/widget/export_project/__init__.py(1 hunks)cosmograph/widget/export_project/create_project.py(1 hunks)cosmograph/widget/export_project/upload_file.py(1 hunks)eslint.config.js(1 hunks)js/legends.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- cosmograph/data/params_ssot.json
🚧 Files skipped from review as they are similar to previous changes (4)
- cosmograph/widget/export_project/upload_file.py
- cosmograph/widget/export_project/create_project.py
- cosmograph/config.py
- cosmograph/widget/export_project/init.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
js/legends.ts (3)
cosmograph/widget/__init__.py (1)
Cosmograph(26-302)js/helper.ts (2)
isDuckDBNumericType(16-32)isDuckDBStringType(34-38)js/components/point-timeline.ts (1)
hide(36-38)
🔇 Additional comments (6)
eslint.config.js (1)
45-45: Good addition of theno-case-declarationsruleAdding this ESLint rule is a valuable improvement to the codebase's quality standards. The rule prevents potential scoping issues by requiring block declarations (using curly braces) around variable declarations in
caseclauses ofswitchstatements. This helps avoid subtle bugs where variables declared in one case might unexpectedly affect other cases.This change aligns well with the broader improvements in code quality and configuration management being implemented throughout the project, particularly as you're adding new functionality for API key management and export features.
js/legends.ts (5)
10-11: Confirm that helper-based type checks suffice
These imports correctly detect DuckDB numeric vs. string columns. Please verify if other column types (e.g. date/time) need to be handled here or if ignoring them is acceptable.
12-18: Enums for readability
Defining string-based enums for color and size strategies, as well as color types, improves code clarity and maintainability.Also applies to: 20-24, 26-29
31-43: Evaluate fallback behavior
If the column type is neither numeric nor string, this function returnsundefined(hiding the legend). Please confirm that defaulting to no legend is the intended behavior for non-numeric, non-string columns.
73-94: Pre-initialization of legends
Creating and hiding all legends upfront simplifies their lifecycle management and is a clean approach.
131-135: Refined show/hide abstraction
A dedicated_updateVisibilityfunction improves readability and avoids repetitive hide/show calls elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
js/widget.ts (1)
202-203: Same lint issue – residual debug output- console.log(`model.get('point_timeline_by')`, model.get('point_timeline_by')) + // console.log removed – use cosmograph.logger.debug if available🧰 Tools
🪛 ESLint
[error] 202-202: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
🧹 Nitpick comments (9)
cosmograph/data/params_ssot.json (4)
8-37: Prefer concrete boolean defaults instead ofnullfor new click/selection flagsFor UX‑oriented flags such as
select_point_on_click,focus_point_on_click, etc., the JSON now sets
"default": nullwhile the annotation isbool. Down‑stream code must treat three states (true/false/null) instead of the usual two, which complicates validation and widget synchronisation. Unless the “unset” state is truly required, consider using an explicitfalsedefault (mirrors the pattern adopted forshow_cluster_labelsbelow).
396-401: Declare the allowed enum values forpoint_size_strategyThe backend/TS code expects
'auto' | 'degree' | 'direct'(seePointSizeStrategyenum).
Adding a"choices"array helps downstream validators & UI builders and prevents silent typos:"annotation": "str", -"description": "Specifies the strategy for sizing points based on data from the point_size_by column." +"choices": ["auto", "degree", "direct"], +"description": "Specifies the strategy for sizing points – • auto: pick automatically • degree: map node degree to size • direct: use raw column values."
541-575: Inconsistent default policy between label‑related flags
show_cluster_labelsdefaults tofalse, whileshow_focused_point_labeland other new label flags default tonull.
Recommend harmonising – pick eitherfalseeverywhere or keepnullbut explain why only cluster labels have a concrete default.
578-605: Validate numeric ranges for label‑weight & spacing parameters
static_label_weight,dynamic_label_weight,label_margin, and the four‑elementlabel_paddingarray influence layout forces. Adding"minimum"/"maximum"(0–1 for weights, ≥0 for spacing) would guard against user mis‑configuration.js/widget.ts (2)
100-107:updatePythonCosmographConfigrecreates the Set on every call
new Set(Object.values(snakeToCamelConfigProps))allocates on each invocation.
Compute it once at module scope:-const updatePythonCosmographConfig = (): void => { - const camelCaseConfigProps = new Set(Object.values(snakeToCamelConfigProps)) +const CAMEL_CONFIG_PROPS = new Set(Object.values(snakeToCamelConfigProps)) +const updatePythonCosmographConfig = (): void => { const filteredConfig = Object.fromEntries( - Object.entries(cosmographConfig).filter(([camelCasePropKey]) => camelCaseConfigProps.has(camelCasePropKey)) + Object.entries(cosmographConfig).filter(([camelCasePropKey]) => CAMEL_CONFIG_PROPS.has(camelCasePropKey)) ) ... }
148-156: Redundantawait cosmograph?.getConfig()
setConfigis synchronous in the Cosmograph API; the extra round‑trip and Promise allocation increase latency for every property change.- cosmograph?.setConfig(cosmographConfig) - // await when config is set - await cosmograph?.getConfig() + cosmograph?.setConfig(cosmographConfig)js/legends.ts (3)
96-99:updateis declaredasyncbut performs no awaitThe method always returns immediately; remove
asyncto avoid misleading callers.- public async update( + public update(
31-43:getPointColorLegendTypeexits early but could memoiseRepeatedly scanning
pointsSummaryon every model change may get expensive with thousands of columns.
Consider caching(column_name → column_type)in a Map atsetCosmographtime.
106-118: Missing block braces forpoint_colorswitch clause variables
colorTypeis scoped to the fullswitch. Wrap the clause like you did forpoint_sizeto avoid accidental leakage.- case 'point_color': { + case 'point_color': { const colorType = ... ... break - } + }(Include the closing brace before
break.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
cosmograph/_dev_utils/data/config_prep/parsed_types.json(1 hunks)cosmograph/base.py(5 hunks)cosmograph/data/params_ssot.json(7 hunks)cosmograph/widget/__init__.py(5 hunks)js/config-props.json(1 hunks)js/legends.ts(2 hunks)js/widget.ts(4 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cosmograph/_dev_utils/data/config_prep/parsed_types.json
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- js/config-props.json
- cosmograph/base.py
- cosmograph/widget/init.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
js/legends.ts (3)
cosmograph/widget/__init__.py (1)
Cosmograph(26-302)js/helper.ts (2)
isDuckDBNumericType(16-32)isDuckDBStringType(34-38)js/components/point-timeline.ts (1)
hide(36-38)
🪛 ESLint
js/widget.ts
[error] 194-194: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
[error] 202-202: Unexpected console statement. Only these console methods are allowed: warn, error.
(no-console)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (3)
cosmograph/data/params_ssot.json (1)
112-116: Description mentions “0 to 1” but default isnull
simulation_cluster_strengthis documented as a float coefficient “from 0 to 1”.
Providingnullas the default leaves the consumer uncertain whethernull⇒ “use Cosmograph auto‑heuristic” or “switch the clustering force off”. Clarify this in the description or pick a sentinel value (e.g.0) and document it.js/widget.ts (1)
55-60: Event name & method now diverge – verify the Python sideYou changed the handler to
setFocusedPoint, but kept the IPC message type as'focus_point'.
Double‑check that the backend still emitsfocus_point(and not the newfocus_point_by_id) to avoid breaking older notebooks.js/legends.ts (1)
131-141: Guard already present – good use of early returnThe null‑check in
_updateVisibilityprevents runtime errors when legends have not yet been initialised. Nice touch.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cosmograph/widget/utils.py (2)
1-11: Good implementation of caching for performance optimizationThe setup for joblib caching is well-structured with a clear location for the cache directory. This will help improve performance for repeated conversions of the same DataFrame.
A few suggestions:
- Consider adding a mechanism to periodically clear the cache or limit its size to prevent unbounded growth.
- Document the cache directory location in project documentation to help users understand where disk space might be used.
13-19: Function documentation could be more comprehensiveThe docstring provides basic information but could be enhanced with parameter and return type descriptions.
Consider expanding the docstring with more details:
@memory.cache def get_buffered_arrow_table(df): """Converts a Pandas DataFrame to a buffered Arrow IPC stream format. This function is cached using joblib.Memory. + + Args: + df (pandas.DataFrame): The DataFrame to convert to Arrow format. + + Returns: + bytes: Buffered Arrow IPC stream as bytes, or None if conversion fails. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)cosmograph/widget/__init__.py(5 hunks)cosmograph/widget/utils.py(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- cosmograph/widget/init.py
🧰 Additional context used
🪛 Ruff (0.8.2)
cosmograph/widget/utils.py
31-31: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (1)
cosmograph/widget/utils.py (1)
23-25: Memory optimization through int64 to int32 downcastingThe downcasting of int64 columns to int32 is a good optimization for memory usage. However, be aware that this may cause data truncation if values exceed int32 range (-2^31 to 2^31-1).
Consider adding a safety check before downcasting:
- df_int32 = df.select_dtypes(include=["int64"]).astype("int32") - df[df_int32.columns] = df_int32 + # Only downcast int64 columns if their values are within int32 range + int64_cols = df.select_dtypes(include=["int64"]) + if not int64_cols.empty: + int32_min, int32_max = -(2**31), (2**31)-1 + can_downcast = ((int64_cols >= int32_min) & (int64_cols <= int32_max)).all().all() + if can_downcast: + df[int64_cols.columns] = int64_cols.astype("int32")
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
cosmograph/widget/export_project/upload_file.py (1)
5-5: Remove unused import.The
jsonimport is not used in this file and should be removed.-import jsoncosmograph/widget/export_project/__init__.py (2)
6-6: Remove unused import.The
jsonimport is not used in this file and should be removed.-import json
82-82: Remove unused variable assignment.The
resultvariable is assigned but never used. Either use the result or remove the assignment.- result = create_project( + create_project( api_key=api_key, project_name=project_name, points_data=points_data, links_data=links_data, cosmograph_config=cosmograph_config, )cosmograph/widget/export_project/create_project.py (1)
5-5: Remove unused import.The
jsonimport is not used in this file and should be removed.-import jsoncosmograph/widget/__init__.py (1)
198-198: Add validation for API key format.Consider adding validation for the API key format to catch invalid keys early and provide better error messages.
-api_key = Unicode(None, allow_none=True) +api_key = Unicode(None, allow_none=True).tag(sync=True) + +@observe("api_key") +def _validate_api_key(self, change): + api_key = change.new + if api_key is not None and not isinstance(api_key, str): + raise ValueError("API key must be a string") + if api_key is not None and len(api_key.strip()) == 0: + raise ValueError("API key cannot be empty")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
cosmograph/widget/__init__.py(9 hunks)cosmograph/widget/export_project/__init__.py(1 hunks)cosmograph/widget/export_project/config.py(1 hunks)cosmograph/widget/export_project/create_project.py(1 hunks)cosmograph/widget/export_project/upload_file.py(1 hunks)js/components/control-buttons.ts(2 hunks)js/config-props.json(1 hunks)js/legends.ts(2 hunks)js/widget.ts(5 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cosmograph/widget/export_project/config.py
- package.json
- js/config-props.json
- js/widget.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
js/components/control-buttons.ts (2)
cosmograph/widget/__init__.py (1)
Cosmograph(26-313)js/widget-elements.ts (1)
createWidgetControlElements(56-79)
js/legends.ts (2)
cosmograph/widget/__init__.py (1)
Cosmograph(26-313)js/helper.ts (2)
isDuckDBNumericType(16-32)isDuckDBStringType(34-38)
🪛 Flake8 (7.2.0)
cosmograph/widget/export_project/__init__.py
[error] 6-6: 'json' imported but unused
(F401)
[error] 21-21: indentation is not a multiple of 4
(E111)
[error] 37-37: indentation is not a multiple of 4
(E111)
[error] 42-42: indentation is not a multiple of 4
(E111)
[error] 43-43: indentation is not a multiple of 4
(E111)
[error] 44-44: continuation line under-indented for visual indent
(E128)
[error] 47-47: indentation is not a multiple of 4 (comment)
(E114)
[error] 48-48: indentation is not a multiple of 4
(E111)
[error] 49-49: indentation is not a multiple of 4
(E111)
[error] 53-53: indentation is not a multiple of 4
(E111)
[error] 57-57: indentation is not a multiple of 4 (comment)
(E114)
[error] 58-58: indentation is not a multiple of 4
(E111)
[error] 59-59: indentation is not a multiple of 4
(E111)
[error] 60-60: indentation is not a multiple of 4
(E111)
[error] 63-63: indentation is not a multiple of 4
(E111)
[error] 64-64: indentation is not a multiple of 4
(E111)
[error] 66-66: indentation is not a multiple of 4
(E111)
[error] 67-67: indentation is not a multiple of 4
(E111)
[error] 69-69: indentation is not a multiple of 4
(E111)
[error] 72-72: indentation is not a multiple of 4
(E111)
[error] 73-73: indentation is not a multiple of 4
(E111)
[error] 75-75: indentation is not a multiple of 4
(E111)
[error] 76-76: indentation is not a multiple of 4
(E111)
[error] 78-78: indentation is not a multiple of 4 (comment)
(E114)
[error] 79-79: indentation is not a multiple of 4
(E111)
[error] 82-82: local variable 'result' is assigned to but never used
(F841)
[error] 82-82: indentation is not a multiple of 4
(E111)
[error] 89-89: indentation is not a multiple of 4 (comment)
(E114)
[error] 91-91: indentation is not a multiple of 4
(E111)
[error] 92-92: indentation is not a multiple of 4
(E111)
[error] 94-94: indentation is not a multiple of 4
(E111)
[error] 96-96: indentation is not a multiple of 4
(E111)
cosmograph/widget/export_project/upload_file.py
[error] 5-5: 'json' imported but unused
(F401)
[error] 11-11: indentation is not a multiple of 4
(E111)
[error] 26-26: indentation is not a multiple of 4
(E111)
[error] 43-43: indentation is not a multiple of 4
(E111)
[error] 45-45: indentation is not a multiple of 4
(E111)
[error] 46-46: indentation is not a multiple of 4
(E111)
[error] 47-47: indentation is not a multiple of 4
(E111)
[error] 48-48: indentation is not a multiple of 4
(E111)
[error] 53-53: indentation is not a multiple of 4
(E111)
[error] 61-61: indentation is not a multiple of 4
(E111)
[error] 66-66: indentation is not a multiple of 4
(E111)
cosmograph/widget/export_project/create_project.py
[error] 5-5: 'json' imported but unused
(F401)
[error] 15-15: indentation is not a multiple of 4
(E111)
[error] 28-28: indentation is not a multiple of 4
(E111)
[error] 48-48: indentation is not a multiple of 4
(E111)
[error] 61-61: indentation is not a multiple of 4
(E111)
[error] 78-78: indentation is not a multiple of 4
(E111)
[error] 79-79: indentation is not a multiple of 4
(E111)
[error] 80-80: indentation is not a multiple of 4
(E111)
[error] 85-85: indentation is not a multiple of 4 (comment)
(E114)
[error] 86-86: indentation is not a multiple of 4
(E111)
[error] 99-99: indentation is not a multiple of 4
(E111)
[error] 104-104: indentation is not a multiple of 4
(E111)
[error] 108-108: indentation is not a multiple of 4
(E111)
[error] 112-112: indentation is not a multiple of 4
(E111)
[error] 116-116: indentation is not a multiple of 4
(E111)
[error] 120-120: indentation is not a multiple of 4
(E111)
[error] 124-124: indentation is not a multiple of 4
(E111)
[error] 128-128: indentation is not a multiple of 4
(E111)
[error] 131-131: indentation is not a multiple of 4
(E111)
[error] 136-136: indentation is not a multiple of 4 (comment)
(E114)
[error] 137-137: indentation is not a multiple of 4
(E111)
[error] 151-151: indentation is not a multiple of 4
(E111)
[error] 152-152: indentation is not a multiple of 4
(E111)
[error] 154-154: indentation is not a multiple of 4 (comment)
(E114)
[error] 155-155: indentation is not a multiple of 4
(E111)
[error] 156-156: indentation is not a multiple of 4
(E111)
[error] 159-159: indentation is not a multiple of 4
(E111)
[error] 160-160: indentation is not a multiple of 4
(E111)
[error] 163-163: indentation is not a multiple of 4 (comment)
(E114)
[error] 164-164: indentation is not a multiple of 4 (comment)
(E114)
[error] 165-165: indentation is not a multiple of 4 (comment)
(E114)
[error] 166-166: indentation is not a multiple of 4 (comment)
(E114)
[error] 168-168: indentation is not a multiple of 4
(E111)
[error] 173-173: indentation is not a multiple of 4
(E111)
[error] 193-193: indentation is not a multiple of 4
(E111)
🪛 Ruff (0.11.9)
cosmograph/widget/export_project/__init__.py
82-82: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (19)
js/components/control-buttons.ts (1)
1-1: Clean dependency upgrade with improved naming.The changes correctly update the button component from
CosmographButtonSelectAreatoCosmographButtonRectangularSelection, which provides better specificity about the selection mode. The constructor signature remains consistent and the change aligns with the cosmograph library upgrade.Also applies to: 10-10, 19-19
cosmograph/widget/export_project/upload_file.py (1)
26-51: Good error handling implementation.The function properly handles both API errors (signed URL generation) and upload errors with appropriate logging and exception chaining. The error messages are informative and the use of
raise ... from epreserves the original exception context.cosmograph/widget/export_project/__init__.py (2)
37-40: Well-implemented error handling.The function properly raises
ValueErrorwith a clear error message when no API key is provided, addressing the previous review comment about consistent error handling approach.
42-96: Comprehensive export workflow implementation.The function orchestrates a well-structured export process with proper error handling at each step:
- Create empty project
- Upload files (points and links)
- Update project with configuration
The logging is informative and the exception handling preserves context with proper chaining.
cosmograph/widget/export_project/create_project.py (3)
78-78: Good defensive programming with config handling.The code properly handles the case where
cosmograph_configisNoneby defaulting to an empty dictionary, addressing the previous review comment about input validation.
94-171: Comprehensive column mapping implementation.The function provides thorough column mapping for both points and links data, handling all the major visualization attributes (id, label, color, size, coordinates, cluster, timeline). The conditional mapping ensures only relevant columns are included in the configuration.
173-196: Robust API integration with proper error handling.The API call is well-structured with proper error handling, logging, and exception chaining. The use of
raise_for_status()and detailed error messages provides good debugging information.js/legends.ts (6)
12-29: Well-defined enums improve type safety.The new enums for
PointColorStrategy,PointSizeStrategy, andColorTypeprovide clear type definitions and improve code readability. The enum values match the expected string literals used throughout the codebase.
31-43: Comprehensive helper function for color legend type determination.The
getPointColorLegendTypefunction centralizes the logic for determining whether to show a range or type color legend. It properly handles different color strategies and uses the DuckDB type helpers to make intelligent decisions based on column data types.
74-96: Improved initialization strategy with safe error handling.The upfront initialization of all legend instances in
setCosmographis a good architectural improvement. The safe hide methods provide a robust workaround for the known cosmograph library bug where_uiComponentmight be undefined.
107-131: Properly scoped switch statement variables.The switch statement now correctly scopes variables within blocks, addressing the previous review comment about variable declarations bleeding between switch clauses. This prevents potential scoping issues and improves code safety.
136-157: Robust error handling for library bugs.The safe show/hide methods provide excellent defensive programming by gracefully handling the known cosmograph library bug. The warning messages help with debugging while preventing the application from crashing.
159-169: Clean visibility management.The
_updateVisibilitymethod is well-structured and uses the safe methods to handle legend visibility changes. The early return for undefined legends prevents unnecessary operations.cosmograph/widget/__init__.py (6)
6-10: LGTM: Clean import organization and good separation of concerns.The new imports are well-organized and serve specific purposes:
Dictimport for type annotations- Utility function for optimized data serialization
- Export functionality for project management
- API key management for global configuration
108-111: LGTM: Consistent configuration pattern for polygonal selection.The polygonal selection configuration follows the established pattern used throughout the class, maintaining consistency with other visual configuration parameters.
169-174: LGTM: Well-designed cluster behavior configuration.The new cluster-related configuration attributes are appropriately named and follow the established patterns for boolean configuration options.
230-240: LGTM: Consistent method implementation for polygonal selection.The new polygonal selection methods follow the established pattern used by other selection methods in the class, maintaining consistency in the API design.
203-203: LGTM: Optimized serialization with caching.The change to use
get_buffered_arrow_tableutility function is a good improvement that likely provides caching and optimization benefits over the previous implementation.Also applies to: 208-208
121-123: point_color_by_map type update is safe
The widget’spoint_color_by_mapnow matches the base API signature incosmograph/base.py(Dict[str, Union[str, list[float]]]), and no other references to this property exist in the codebase.No further action required.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
js/legends.ts(2 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ 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). (3)
- GitHub Check: Validation (3.10)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (4)
js/legends.ts (4)
62-87: Excellent refactor: Pre-instantiated legends with consistent configuration.The upfront legend instantiation approach is much cleaner than the previous async pattern. The consistent configuration across all legend types (disabled messages, proper labels) improves maintainability and user experience.
100-121: Good fix: Switch statement variable scoping is properly addressed.The previous scoping issue has been correctly resolved by wrapping variable declarations in block scope with curly braces. This prevents variable bleed between switch clauses.
128-149: Robust error handling: Safe legend methods are well-implemented.The try-catch blocks with warning logs provide a good workaround for the cosmograph library bug. The documentation clearly explains the purpose of these methods.
151-165: Clean implementation: Visibility method uses safe operations.The updated method correctly uses the safe show/hide methods and properly toggles container display styles. The early return for undefined legends is a good defensive programming practice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
160-167: Avoid hard-coding secrets in documentation examplesShowing the API key as a raw string encourages insecure practices. Prefer an environment variable to prevent accidental key leaks in notebooks committed to VCS:
-from cosmograph import set_api_key - -# Set your API key globally (this will apply to all cosmograph instances) -set_api_key("your-api-key-here") +import os +from cosmograph import set_api_key + +# Recommended: keep secrets outside source control +set_api_key(os.environ["COSMOGRAPH_API_KEY"])
176-209: Call out optional dependencies and return value forexport_project_by_name
- The export path writes Parquet; users need
pyarroworfastparquet. Add a short note orpip install "cosmograph[export]"hint to prevent runtime surprises.- Clarify whether
export_project_by_name()is synchronous and what it returns (URL? project ID?) so readers know how to proceed after the call.A one-liner right after the code block would suffice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
With help from CodeRabbitAI
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cosmograph/widget/__init__.py (2)
296-305: Well-implemented export method with proper validation.The implementation properly addresses the previous review comments by including:
- API key validation
- Project name validation
- Comprehensive error handling with context
- Return value from export_project
306-331: Excellent implementation of API key management with proper error handling.The implementation successfully addresses all previous review concerns:
- Registration failures are caught and logged without failing initialization
- Unregistration errors are properly ignored during cleanup
- The
super().__del__()call is protected with a hasattr check
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cosmograph/widget/__init__.py(9 hunks)cosmograph/widget/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cosmograph/widget/utils.py
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (2)
cosmograph/widget/__init__.py (2)
6-10: Imports align with new functionality.The imports are properly organized and support the new API key management and export capabilities.
121-123: Property type updates are consistent with the overall design.The change from nested list to dictionary for
point_color_by_mapimproves type clarity and aligns with the updates mentioned in the AI summary.Also applies to: 157-157
With help from CodeRabbitAI - Add null safety for cosmograph.stats access in getPointColorLegendType - Add polygon parameter validation in select_points_in_polygon method
Stukova
changed the title
…lay features - Add cluster labeling support with customizable colors and fonts - Implement polygonal selector styling options (stroke color, line width) - Add cluster position mapping for custom cluster placement - Enhance point/cluster selection behavior with granular controls - Add display state management and status indicator modes - Support link scaling on zoom and position preservation on data updates - Rename simulation_cluster to simulation_cluster_strength - Clean up widget configuration and remove unused properties - Update demo with new cluster label features
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cosmograph/data/params_ssot.json (1)
711-721: Expose clicked_cluster in SSOT to match widget eventwidget.ts sets model.set('clicked_cluster', cluster) on onClusterLabelClick. This key isn’t declared here; adding it keeps the SSOT truly “single source” and avoids surprises in trait/serialization layers.
Add this entry alongside other interaction outputs:
{ "name":"changeLinks", "default":null, "annotation":"typing.Callable[[typing.Dict[str, typing.Any]], typing.Any]", "description":"" }, + { + "name":"clicked_cluster", + "default": null, + "annotation":"typing.Any", + "description":"The cluster key of the label that was clicked (emitted by onClusterLabelClick)." + },cosmograph/base.py (1)
188-206: All code references updated; only docs/examples remainThe rename of
simulation_cluster→simulation_cluster_strengthand ofscale_links→scale_links_on_zoomis correctly reflected incosmograph/base.py, the widget (__init__.py),js/config-props.jsonanddata/params_ssot.json. No stale uses were found in the core library.Remaining occurrences of the old
simulation_clusterlive in example notebooks undermisc/anddemos/. Please update those to avoid confusion and broken examples:
- misc/defining_ssot_for_config_properties.ipynb (cells around line 139, 199, 245, 2662, 2883, 3106, 3521, 3589, 3674)
- demos/colab_Clusters_in_Cosmograph_Widget.ipynb
- demos/colab_Mobius_in_Cosmograph_Widget.ipynb
- demos/colab_English_Words.ipynb
- demos/colab_Cosmograph_Widget_Timeline.ipynb
No further action required for
scale_links_on_zoom.
🧹 Nitpick comments (19)
cosmograph/data/params_ssot.json (7)
45-49: Prefer “None” (Python) over “undefined” (JS) in descriptionsDocs in a Python SSOT should use Python terminology to avoid confusion.
- "description":"Controls display state messages behavior for all Cosmograph components. When `undefined`: Show all state messages (loading, no data). When `'loading'`: Show only loading state message, hide no data message. When `false`: Disable all state messages. This will be applied to all components unless explicitly overridden in component-specific configuration. @default `undefined`" + "description":"Controls display state messages behavior for all Cosmograph components. When `None` (not set): show all state messages (loading, no data). When `'loading'`: show only the loading state message and hide the no-data message. When `False`: disable all state messages. This will be applied to all components unless explicitly overridden in component-specific configuration. @default None"
132-136: Clarify “None” vs “undefined” in descriptionStay consistent with Python vocabulary and clarify behavior.
- "description":"Coefficient of simulation cluster strength from 0 to 1. If `undefined`, the graph will not apply any clustering force to the simulation." + "description":"Coefficient of simulation cluster strength from 0 to 1. If not set (`None`), the graph will not apply any clustering force to the simulation."
87-91: Fix typo in description (“detalization”)Minor wording fix for professionalism.
- "description":"Decreases / increases the detalization of the Many-Body force calculations." + "description":"Decreases/increases the detail level of the Many-Body force calculations."
93-97: Type annotation should be int for levelsQuadtree “levels” is a count; using int is clearer and semantically correct.
- "annotation":"float", + "annotation":"int",
465-469: Tighten wording and fix “centermass”Also clarify behavior when pointXBy/pointYBy provided.
- "description":"Mapping of cluster keys to [x, y] coordinate positions. Keys should match values from the pointClusterBy column. Missing cluster keys will be automatically positioned using the centermass. Won't take effect if point positions are provided by pointXBy or pointYBy. @example Object mapping cluster keys to coordinates: clusterPositionsMap: { 'Rock': [10, 20], 'Rap': [30, 40] }" + "description":"Mapping of cluster keys to [x, y] coordinates. Keys should match values from the `pointClusterBy` column. Missing keys are positioned using the center of mass. Ignored if explicit point positions are provided via `pointXBy`/`pointYBy`. Example: { 'Rock': [10, 20], 'Rap': [30, 40] }"
258-262: Fill in link_arrows descriptionEmpty descriptions hinder usability.
- "description":"" + "description":"Enables directional arrows on links when True. When False or None, no arrows are rendered."
8-19: Optional: clarify accepted color formats for polygonal selector strokeState whether CSS color strings (e.g., “#RRGGBB”, “rgb(...)”, CSS color names) or RGBA arrays are accepted, to match other color params’ descriptions.
js/widget.css (3)
26-37: Ensure positioned ancestor for absolutely positioned children.bottom is absolutely positioned. If no ancestor is positioned, it will anchor to the page, not the graph. Consider making .wrapper .graph (or a containing element) position: relative to scope children correctly.
.wrapper .graph { flex: 1; /* height: 100%; */ min-height: 0; display: flex; + position: relative; }
66-71: Backdrop filter may be costly and lacks universal supportbackdrop-filter: blur(6px) can impact performance and isn’t supported in all environments. Consider a guarded class or fallback.
81-83: Remove empty rule set or add a commentEmpty .controls block is noise unless intentionally reserved for future overrides.
-.wrapper .controls { - -} +/* .wrapper .controls reserved for legacy styles – keep empty intentionally */js/widget.ts (1)
154-165: Async handler is not awaited; safe here but be mindful of sequencingonModelChange is async for all mapped props, but not awaited before setConfig. Currently harmless since it only reads model values and updates an object. If you later add async work (e.g., network, file IO), sequence could get racy.
cosmograph/base.py (1)
321-326: Use the global API key when api_key is empty string (not just None).Current check only handles None; an empty string will bypass the fallback and later fail in export. Treat falsey string as missing to be more ergonomic.
Apply this small diff:
- # If api_key is None, use the global one if available - if kwargs.get('api_key') is None: - global_api_key = get_api_key() - if global_api_key is not None: - kwargs['api_key'] = global_api_key + # If api_key is empty or None, use the global one if available + api_key = kwargs.get("api_key") + if not api_key: + global_api_key = get_api_key() + if global_api_key: + kwargs["api_key"] = global_api_keycosmograph/widget/__init__.py (7)
120-123: point_color_by_map trait updated to Dict with correct inner types.The Dict key/value traits and Union look good. Note: elsewhere in the file both List(Float) and List(Float()) patterns are used; consider unifying style later.
214-214: Avoid mutable default for cosmograph_config.Using {} as a default can lead to shared mutable state across instances. Traitlets often copies defaults, but safer to use a dynamic default.
Apply this diff for the attribute line and add a dynamic default method (shown below):
- cosmograph_config = Dict(default_value={}, allow_none=True).tag(sync=True) + cosmograph_config = Dict(default_value=None, allow_none=True).tag(sync=True)Add this method inside the class to provide an instance-specific default:
def _cosmograph_config_default(self): return {}
219-227: Arrow IPC conversion: beware silent int64→int32 downcasts.get_buffered_arrow_table currently downcasts int64 columns to int32, which can overflow silently for large IDs. If your point/link IDs or indices can exceed int32 range, consider preserving int64 or making downcasting opt-in.
Proposed adjustment in cosmograph/widget/utils.py (outside this diff):
def get_buffered_arrow_table(df): if df is None: return None try: # Optional: only downcast if values fit, otherwise keep int64 import numpy as np df2 = df.copy() for col in df2.select_dtypes(include=["int64"]).columns: s = df2[col] if s.min(skipna=True) >= np.iinfo(np.int32).min and s.max(skipna=True) <= np.iinfo(np.int32).max: df2[col] = s.astype("int32") table = pa.Table.from_pandas(df2, preserve_index=False) sink = pa.BufferOutputStream() with pa.ipc.new_stream(sink, table.schema) as writer: writer.write(table) return sink.getvalue().to_pybytes() except Exception as e: logger.warning("Failed to convert DataFrame to buffered Arrow table: %s", str(e)) return None
248-264: Polygon selection: accept tuple containers too.You already accept list/tuple for points; accept tuple for the polygon container for symmetry.
Apply this diff:
- def select_points_in_polygon(self, polygon): - if not isinstance(polygon, list) or len(polygon) < 3: + def select_points_in_polygon(self, polygon): + if not isinstance(polygon, (list, tuple)) or len(polygon) < 3: raise ValueError("Polygon must be a list of at least 3 coordinate pairs") for point in polygon: if not isinstance(point, (list, tuple)) or len(point) != 2: raise ValueError("Each polygon point must be a coordinate pair [x, y]") self.send({"type": "select_points_in_polygon", "polygon": polygon})
319-327: Export: consider falling back to global API key at call-time.If users unset self.api_key after init, the export will fail even if a global key is set. Safe fallback improves UX without weakening validation.
Apply this diff:
- def export_project_by_name(self, project_name: str): - if not self.api_key: + def export_project_by_name(self, project_name: str): + api_key = self.api_key or get_api_key() + if not api_key: raise ValueError("API key is required for project export") if not project_name or not project_name.strip(): raise ValueError("Project name is required and cannot be empty") try: - return export_project(self.api_key, project_name, self.points, self.links, self.cosmograph_config) + return export_project(api_key, project_name, self.points, self.links, self.cosmograph_config) except Exception as e: raise RuntimeError(f"Failed to export project '{project_name}': {str(e)}") from eAlso, note that export_project currently returns a project_id (str) — consider aligning its type annotation/docstring with that return type.
329-343: Prefer logging over print for registration warnings.Printing to stdout is noisy in notebooks and harder to manage in larger apps.
Apply this diff:
- except Exception as e: - # Log warning but don't fail initialization - print(f"Warning: Failed to register instance for API key updates: {e}") + except Exception as e: + # Log warning but don't fail initialization + import logging + logging.getLogger(__name__).warning( + "Failed to register instance for API key updates: %s", e + )
344-353: Consider weakref.finalize instead of del for deterministic cleanup.del can be unreliable at interpreter shutdown and can extend object lifetimes. weakref.finalize provides safer finalization without depending on GC timing.
Apply this diff to remove del:
- def __del__(self): - # Clean up by unregistering when the instance is deleted - try: - unregister_instance(self) - except Exception: - # Ignore errors during cleanup - pass - # Only call super().__del__() if it exists - if hasattr(super(), '__del__'): - super().__del__()And adjust init to register a finalizer (within the same class):
def __init__(self, **kwargs): super().__init__(**kwargs) # Register this instance to receive API key updates try: register_instance(self) except Exception as e: # Log warning but don't fail initialization print(f"Warning: Failed to register instance for API key updates: {e}") # If no API key was provided but global one exists, use it if self.api_key is None: global_api_key = get_api_key() if global_api_key is not None: self.api_key = global_api_key + # Ensure cleanup without relying on __del__ + try: + import weakref + weakref.finalize(self, lambda inst_ref: unregister_instance(inst_ref()) if inst_ref() else None, weakref.ref(self)) + except Exception: + # Best-effort; ignore finalizer setup failures + passAdd at the top of the file if not present:
import weakref # optional: if you prefer a direct import rather than inside __init__
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
cosmograph/base.py(6 hunks)cosmograph/data/params_ssot.json(9 hunks)cosmograph/widget/__init__.py(10 hunks)demos/colab_Clusters_in_Cosmograph_Widget.ipynb(4 hunks)js/config-props.json(1 hunks)js/widget.css(2 hunks)js/widget.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
js/widget.ts (1)
js/helper.ts (1)
subscribe(11-14)
cosmograph/widget/__init__.py (3)
cosmograph/widget/utils.py (1)
get_buffered_arrow_table(17-36)cosmograph/widget/export_project/__init__.py (1)
export_project(14-92)cosmograph/config.py (3)
register_instance(33-40)unregister_instance(43-51)get_api_key(23-30)
cosmograph/base.py (1)
cosmograph/config.py (1)
get_api_key(23-30)
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (27)
js/config-props.json (1)
1-116: Good centralization of snake_case → camelCase mappingsConsolidating these mappings in JSON simplifies maintenance and keeps widget.ts lean. The new keys (e.g., components_display_state_mode, status_indicator_mode, preserve_point_positions_on_data_update, cluster_positions_map, scale_links_on_zoom) align with params_ssot.json.
cosmograph/data/params_ssot.json (3)
627-631: Confirm label_padding orderThe order [left, top, right, bottom] differs from common CSS conventions (top, right, bottom, left). If this order is intentional, consider stating it explicitly to avoid misconfiguration.
291-295: Rename alignment: show_fps_monitor vs show_FPS_monitorThis SSOT uses show_fps_monitor (good). The demo notebook’s help(cosmo) still shows show_FPS_monitor, suggesting a stale signature/docstring. Please align the Python surface.
Use the same script in the earlier comment to locate stale names.
723-727: LGTM: api_key parameter docsClear description that the key can be set via set_api_key() without storing secrets here. Fits the SSOT’s purpose.
demos/colab_Clusters_in_Cosmograph_Widget.ipynb (2)
126-156: help(cosmo) output is stale vs. new public APIIt still shows:
- simulation_cluster (old) instead of simulation_cluster_strength
- show_FPS_monitor (old) instead of show_fps_monitor
- show_top_labels_by (removed)
- point_color_by_map typed as list[list[...]] (now Dict[str, Union[str, list[float]]])
Please regenerate/update the Python signature/docstring so the notebook reflects the new API.
103-113: Nice showcase of new cluster label controlsThe example highlights show_cluster_labels, use_point_color_strategy_for_cluster_labels, and select_cluster_on_label_click well.
js/widget.ts (2)
101-106: Ensure Python model exposes clicked_clusterYou’re emitting model.set('clicked_cluster', cluster) on onClusterLabelClick, but params_ssot.json (SSOT) doesn’t currently include clicked_cluster. Please add it on the Python side (trait) and SSOT to avoid serialization warnings and keep the API discoverable.
196-203: Synchronous legend updates on onGraphRebuilt look goodNice simplification and ensures legends reflect the latest graph state immediately after rebuild.
cosmograph/base.py (5)
16-17: Global API key import is appropriate and scoped correctly.This aligns base with the new global API key management. No issues.
146-156: New polygonal selector and clustering params are consistent with widget API.
- polygonal_selector_stroke_color / polygonal_selector_line_width
- point_size_strategy
- cluster_positions_map
These match new traitlets in the widget. No concerns.
224-239: New labeling/selectability controls LGTM.Adds show_cluster_labels, label font sizes, label padding, point/label click/focus behavior, and display/status modes. The surface matches widget traits and SSOT evolution.
243-243: api_key as a first-class parameter: good addition.This enables per-instance overrides and plays well with the global fallback.
138-139: point_color_by_map consistency verified across all layersAll occurrences of
point_color_by_maphave been updated to useDict[str, Union[str, list[float]]]and are in sync:
- js/config-props.json (line 68):
"point_color_by_map": "pointColorByMap"- cosmograph/base.py (lines 138–139): parameter signature
- cosmograph/data/params_ssot.json (line 399): SSOT annotation
- cosmograph/widget/init.py (line 120): widget trait definition
No further changes required.
cosmograph/widget/__init__.py (14)
6-11: Imports and module wiring for utils/export/config look solid.Brings in the required traits, buffered Arrow converter, export pipeline, and API-key lifecycle hooks. No issues.
49-49: simulation_cluster_strength added as a synced trait — OK.Matches the rename in the Python API and SSOT.
70-70: show_focused_point_label added — consistent with UI behavior.
92-92: scale_links_on_zoom exposed — matches config expansion.
108-111: Polygonal selection controls added.Interfaces for the polygonal selector look correct and map to frontend behavior.
127-127: point_size_strategy added — OK.
134-136: cluster_positions_map added — OK.Value trait as List(Float()) is appropriate.
159-159: show_labels_for default None — OK.Matches the doc surface and keeps inactive by default.
165-171: Cluster label controls and label_padding added — OK.These mirror the new UI affordances.
174-187: Selection/click/focus and components/status modes — OK.The new booleans and Union traits look correct and should round-trip to JS cleanly.
188-193: Advanced options added — OK.status_indicator_mode and preserve_point_positions_on_data_update reflect the new config surface.
211-211: clicked_cluster synced to Python — OK.Useful for downstream interactions.
216-216: api_key stored on the Python side only — good for not leaking secrets to the frontend.
262-264: unselect_points_by_indices passthrough — OK.
| if (msg.type === 'unselect_points_by_indices') { | ||
| cosmograph?.unselectPointsByIndicies(msg.indices) | ||
| } |
There was a problem hiding this comment.
Typo: unselectPointsByIndicies → unselectPointsByIndices (runtime error)
Cosmograph API uses “Indices”. Current call will fail at runtime.
- if (msg.type === 'unselect_points_by_indices') {
- cosmograph?.unselectPointsByIndicies(msg.indices)
- }
+ if (msg.type === 'unselect_points_by_indices') {
+ cosmograph?.unselectPointsByIndices(msg.indices)
+ }📝 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 (msg.type === 'unselect_points_by_indices') { | |
| cosmograph?.unselectPointsByIndicies(msg.indices) | |
| } | |
| if (msg.type === 'unselect_points_by_indices') { | |
| cosmograph?.unselectPointsByIndices(msg.indices) | |
| } |
🤖 Prompt for AI Agents
In js/widget.ts around lines 50 to 52, there's a typo calling
cosmograph?.unselectPointsByIndicies which will cause a runtime error because
the Cosmograph API method is named unselectPointsByIndices; update the method
call to cosmograph?.unselectPointsByIndices(msg.indices) so the correct API is
invoked (and run tests/build to verify no other typos).
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 (2)
js/widget.ts (2)
20-21: Type ofmsgis incorrect ({ [key: string]: never }) — breaks type-checking.Accessing
msg.typeet al. is incompatible withneverand will failtsc. Useunknown/Record<string, unknown>as a quick fix or define a discriminated union for correctness. Also, bind a named handler so you can unbind on dispose.Minimal change:
- model.on('msg:custom', async (msg: { [key: string]: never }) => { + model.on('msg:custom', async (msg: Record<string, unknown>) => {Recommended typed approach (additions outside this range):
// Near imports type WidgetMessage = | { type: 'select_point_by_index'; index: number } | { type: 'select_point_by_id'; id: string | number } | { type: 'select_points_by_indices'; indices: number[] } | { type: 'select_points_by_ids'; ids: Array<string | number> } | { type: 'activate_rect_selection' } | { type: 'deactivate_rect_selection' } | { type: 'activate_polygonal_selection' } | { type: 'deactivate_polygonal_selection' } | { type: 'select_points_in_polygon'; polygon: Array<[number, number]> } | { type: 'unselect_points_by_indices'; indices: number[] } | { type: 'fit_view' } | { type: 'fit_view_by_indices'; indices: number[]; duration?: number; padding?: number } | { type: 'fit_view_by_ids'; ids: Array<string | number>; duration?: number; padding?: number } | { type: 'fit_view_by_coordinates'; coordinates: [[number, number], [number, number]]; duration?: number; padding?: number } | { type: 'focus_point_by_index'; index?: number } | { type: 'focus_point'; id: string | number } | { type: 'start'; alpha?: number } | { type: 'pause' } | { type: 'restart' } | { type: 'step' } | { type: 'capture_screenshot' } // Named handler + proper cleanup const onCustomMessage = async (msg: WidgetMessage): Promise<void> => { // ... current switch-like if/else body ... } model.on('msg:custom', onCustomMessage)And ensure disposal unbinds the handler (see next comment).
225-228: Ensure themsg:customlistener is unbound on cleanup (memory leak).The current anonymous listener isn’t removed, so multiple renders will accumulate handlers. Bind a named handler and unbind here.
Example:
return (): void => { unsubscribes.forEach(unsubscribe => unsubscribe()) + // Unbind the custom message listener + model.off('msg:custom', onCustomMessage) cosmograph?.destroy() }Note: This assumes you introduced
const onCustomMessage = (...) => { ... }as suggested above.
♻️ Duplicate comments (1)
js/widget.ts (1)
50-52: Fix typo in Cosmograph API call: Indicies → Indices (runtime error).This will throw at runtime because Cosmograph exposes
unselectPointsByIndices, not...Indicies.Apply:
- if (msg.type === 'unselect_points_by_indices') { - cosmograph?.unselectPointsByIndicies(msg.indices) - } + if (msg.type === 'unselect_points_by_indices') { + cosmograph?.unselectPointsByIndices(msg.indices) + }
🧹 Nitpick comments (2)
js/widget.ts (2)
156-166: Coalesce frequent setConfig calls to avoid config thrashing.On rapid consecutive model changes, each handler calls
setConfigfollowed bygetConfig(). Coalescing updates per microtask (or animation frame) reduces redundant work and UI jitter.Apply within this block:
- if (snakeToCamelConfigProps[snakeCaseProp as keyof typeof snakeToCamelConfigProps]) { - cosmograph?.setConfig(cosmographConfig) - // await when config is set - await cosmograph?.getConfig() - updatePythonCosmographConfig() - } + if (snakeToCamelConfigProps[snakeCaseProp as keyof typeof snakeToCamelConfigProps]) { + scheduleApplyConfig() + }Then add (outside this range, near other locals) to implement the scheduler:
// Coalesce Cosmograph config applications per microtask let applyConfigQueued = false const scheduleApplyConfig = (): void => { if (!cosmograph || applyConfigQueued) return applyConfigQueued = true queueMicrotask(() => { applyConfigQueued = false cosmograph!.setConfig(cosmographConfig) // If you truly need to await internal settling, prefer requestAnimationFrame here. updatePythonCosmographConfig() }) }
207-216: Minor DRY: consolidate undefined-pruning logic.You already exclude
undefinedfrom the Python-sync surface (Line 112). You can avoid this second pass by extracting a small helper and reusing it both before instantiation and during model-driven updates.For example:
function pruneUndefined<T extends object>(obj: T): T { for (const key of Object.keys(obj) as (keyof T)[]) { if (obj[key] === undefined) delete obj[key] } return obj } // Usage: pruneUndefined(cosmographConfig) updatePythonCosmographConfig()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
js/widget.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
js/widget.ts (1)
js/helper.ts (1)
subscribe(11-14)
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (6)
js/widget.ts (6)
41-49: Polygonal selection wiring looks correct.The message-to-API mapping for polygonal selection aligns with the renamed v2 methods.
66-72: Focus API rename correctly adopted.Using
setFocusedPointfor both index and id-based focus paths is consistent with v2. Good change.
101-106: Good: cluster label click callback is wired and state-synced.Event is persisted via
clicked_clusterandsave_changes(); this matches the new public API surface.
108-117: Nice: filtered config sync excludes undefineds and off-spec keys.This implements the earlier refactor to avoid leaking
undefinedinto JSON and limits the synced surface to known props.
198-205: Legend refresh on graph rebuild is consistent with v2’sonGraphRebuilt.Centralizing legend updates and timeline accessor refresh here is clean.
31-34: I wasn’t able to locate any.d.tsfor@cosmograph/cosmographin this workspace. Could you please share the relevant TypeScript declarations (or a GitHub/npm link) for v2.0.0-beta.25—specifically the signatures of:
selectPoints(...)selectPointsInPolygon(...)unselectPointsByIndices(...)With those I can confirm whether
selectPointsacceptsnullor if you should switch to passing[].
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cosmograph/__about__.py(1 hunks)
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (1)
cosmograph/__about__.py (1)
3-3: Major bump to 1.0.0 pre-release is appropriate given the API surface changes.Given the breaking API updates (global API key, param renames, export pipeline), moving from 0.x to 1.0.0 beta looks justified.
cosmograph/__about__.py
Outdated
| """Package metadata for cosmograph""" | ||
|
|
||
| __version__ = "0.0.47" | ||
| __version__ = "1.0.0b0" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Double‑check PEP 440 compliance of “b0”, packaging single source of truth, and install docs.
- PEP 440 pre-release numbering is slightly nuanced; “b1” is more common than “b0”. Please verify that “1.0.0b0” parses as expected in your publishing pipeline and that tools (pip, twine, packaging) accept it.
- Ensure your build config sources the version from this file (to avoid drift). If pyproject uses a literal version, keep it in sync or switch to dynamic version = attr.
- Since pre-releases aren’t installed by default with “pip install cosmograph”, update README/docs to include installation guidance for testers (e.g., “pip install --pre cosmograph” or pin to 1.0.0b0), especially since this PR advertises new APIs.
If you prefer to avoid any edge cases and align with common practice, you can bump to b1:
-__version__ = "1.0.0b0"
+__version__ = "1.0.0b1"Run this script to validate PEP 440 compliance, detect other version declarations, and scan docs for install guidance:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==> Extract and validate __version__ with packaging"
python - <<'PY'
import re, sys, pathlib
from packaging.version import Version, InvalidVersion
p = pathlib.Path('cosmograph/__about__.py')
text = p.read_text(encoding='utf-8')
m = re.search(r'__version__\s*=\s*[\'"]([^\'"]+)[\'"]', text)
ver = m.group(1) if m else None
print(f"__about__.__version__ = {ver}")
try:
v = Version(ver)
print(f"PEP 440 normalized: {v.public}")
except InvalidVersion as e:
print(f"Invalid PEP 440 version: {e}", file=sys.stderr); sys.exit(1)
from packaging.version import Version
prev = Version("0.0.47")
if v <= prev:
print("Version does not bump above previous 0.0.47", file=sys.stderr); sys.exit(2)
print("Ordering vs previous OK.")
PY
echo
echo "==> Check for other version sources in packaging configs"
rg -nP -C1 '^\s*version\s*=\s*|tool\.setuptools\.dynamic|dynamic\s*=\s*\[.*version.*\]|attr:\s*.*__version__' pyproject.toml setup.cfg setup.py 2>/dev/null || true
echo
echo "==> Scan docs/README for pre-release install hints or stale versions"
rg -nP -C1 '\bpip install cosmograph\b|\b--pre\b|0\.0\.47\b|1\.0\.0b0\b' README.md docs/**/*.md 2>/dev/null || trueLength of output: 925
Confirm version bump and update installation docs
- ✅ version “1.0.0b0” is valid under PEP 440 and correctly orders after 0.0.47.
- ✅ pyproject.toml is configured with
dynamic = ["version"], so the single source of truth iscosmograph/__about__.py.
Action required: update README.md and any docs to include pre-release install instructions. For example, in README.md around line 20:
--- README.md
+++ README.md
@@ -19,3 +19,7
-```sh
-pip install cosmograph
-```
+```sh
+pip install cosmograph
+
+# To install the beta release:
+pip install --pre cosmograph # latest beta (e.g. 1.0.0b0)
+pip install cosmograph==1.0.0b0 # pin to this exact beta
+```- 🔧 Optional: if you’d rather align with more common pre-release numbering, bump to
1.0.0b1.
🤖 Prompt for AI Agents
In cosmograph/__about__.py at line 3 the version is set to "1.0.0b0"; update
README.md (around line 20) and any user-facing docs to include pre-release
install instructions by adding a fenced shell code block showing (1) the normal
pip install command, (2) how to install the beta with pip install --pre
cosmograph, and (3) how to pin to the exact beta with pip install
cosmograph==1.0.0b0; if you prefer a different pre-release tag, optionally
change the version here to "1.0.0b1" and reflect that exact version in the docs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cosmograph/base.py (1)
488-492: Consider extracting API key injection logic.The same API key injection pattern appears in both
cosmo(lines 181-184) andcosmo_advanced(lines 488-492). Sincecosmo_advancedis deprecated, the current duplication is acceptable. However, if other functions need this logic in the future, consider extracting it to a helper function.Example helper function:
def _inject_global_api_key(kwargs: dict) -> dict: """Inject global API key if not provided in kwargs.""" if kwargs.get('api_key') is None: global_api_key = get_api_key() if global_api_key is not None: kwargs['api_key'] = global_api_key return kwargscosmograph/data/params_ssot.json (1)
258-261: Consider adding descriptions for remaining parameters.Several parameters still have empty descriptions:
link_arrows(line 261)point_id_by(line 378)point_index_by(line 384)point_size_by(line 414)point_size_range(line 420)While these can be inferred from their names, adding descriptions would improve documentation completeness and help users understand expected values and behavior.
Also applies to: 375-385, 411-420
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cosmograph/base.py(10 hunks)cosmograph/data/params_ssot.json(9 hunks)cosmograph/util.py(2 hunks)js/config-props.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/config-props.json
🧰 Additional context used
🧬 Code graph analysis (1)
cosmograph/base.py (3)
cosmograph/util.py (1)
add_cosmo_param_descriptions(111-135)cosmograph/config.py (1)
get_api_key(23-30)cosmograph/widget/__init__.py (1)
Cosmograph(26-353)
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (5)
cosmograph/base.py (2)
181-184: LGTM! Global API key injection implemented correctly.The logic properly checks if
api_keyis None before injecting the global value, preventing unintended overwrites of explicitly provided keys.
421-444: LGTM! Deprecation warning properly implemented.The deprecation warning follows Python best practices:
- Uses
warnings.warnfrom the standard library- Specifies
DeprecationWarningclass- Sets
stacklevel=2to point to the caller- Provides clear migration guidance
cosmograph/data/params_ssot.json (3)
8-61: LGTM! New selection and focus parameters well-documented.The new parameters (
polygonal_selector_stroke_color,polygonal_selector_line_width,select_point_on_click,focus_point_on_click,components_display_state_mode,status_indicator_mode,preserve_point_positions_on_data_update) all have clear, detailed descriptions that explain their purpose and behavior.
573-649: LGTM! Label configuration parameters thoroughly documented.The new label-related parameters provide comprehensive control over label appearance and behavior. All parameters include helpful descriptions explaining their purpose and default behavior.
710-715: LGTM! API key parameter properly documented.The
api_keyparameter now has a clear description that explains its purpose and mentions the global configuration option viaset_api_key(), addressing the previous review feedback.
| def add_cosmo_param_descriptions(func, param_names=None): | ||
| """ | ||
| Decorator to add parameter descriptions from SSOT data to cosmo function. | ||
| Simply appends SSOT parameter descriptions to the existing Args section. | ||
| Args: | ||
| func: The cosmo function to decorate. | ||
| param_names: Optional list of param names to filter SSOT (uses _params_ssot). | ||
| """ | ||
| params_ssot = _params_ssot(param_names) | ||
| param_docs = {p["name"]: p for p in params_ssot} | ||
|
|
||
| sig = inspect.signature(func) | ||
| relevant_params = [ | ||
| p for name, p in param_docs.items() | ||
| if name in sig.parameters and p["description"] | ||
| ] | ||
|
|
||
| if relevant_params: | ||
| # Simply append SSOT parameters to the existing docstring | ||
| ssot_docs = "\n".join(f" {p['name']}: {p['description']}" for p in relevant_params) | ||
| func.__doc__ = func.__doc__.rstrip() + "\n" + ssot_docs + "\n" | ||
|
|
||
| return func | ||
|
|
There was a problem hiding this comment.
Add None check for function docstring.
The decorator assumes func.__doc__ is not None. If the decorated function lacks a docstring, line 133 will raise an AttributeError when calling .rstrip() on None.
Apply this diff to handle functions without docstrings:
def add_cosmo_param_descriptions(func, param_names=None):
"""
Decorator to add parameter descriptions from SSOT data to cosmo function.
Simply appends SSOT parameter descriptions to the existing Args section.
Args:
func: The cosmo function to decorate.
param_names: Optional list of param names to filter SSOT (uses _params_ssot).
"""
params_ssot = _params_ssot(param_names)
param_docs = {p["name"]: p for p in params_ssot}
sig = inspect.signature(func)
relevant_params = [
p for name, p in param_docs.items()
if name in sig.parameters and p["description"]
]
if relevant_params:
+ # Handle functions without docstrings
+ if func.__doc__ is None:
+ func.__doc__ = ""
+
# Simply append SSOT parameters to the existing docstring
ssot_docs = "\n".join(f" {p['name']}: {p['description']}" for p in relevant_params)
func.__doc__ = func.__doc__.rstrip() + "\n" + ssot_docs + "\n"
return func📝 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 add_cosmo_param_descriptions(func, param_names=None): | |
| """ | |
| Decorator to add parameter descriptions from SSOT data to cosmo function. | |
| Simply appends SSOT parameter descriptions to the existing Args section. | |
| Args: | |
| func: The cosmo function to decorate. | |
| param_names: Optional list of param names to filter SSOT (uses _params_ssot). | |
| """ | |
| params_ssot = _params_ssot(param_names) | |
| param_docs = {p["name"]: p for p in params_ssot} | |
| sig = inspect.signature(func) | |
| relevant_params = [ | |
| p for name, p in param_docs.items() | |
| if name in sig.parameters and p["description"] | |
| ] | |
| if relevant_params: | |
| # Simply append SSOT parameters to the existing docstring | |
| ssot_docs = "\n".join(f" {p['name']}: {p['description']}" for p in relevant_params) | |
| func.__doc__ = func.__doc__.rstrip() + "\n" + ssot_docs + "\n" | |
| return func | |
| def add_cosmo_param_descriptions(func, param_names=None): | |
| """ | |
| Decorator to add parameter descriptions from SSOT data to cosmo function. | |
| Simply appends SSOT parameter descriptions to the existing Args section. | |
| Args: | |
| func: The cosmo function to decorate. | |
| param_names: Optional list of param names to filter SSOT (uses _params_ssot). | |
| """ | |
| params_ssot = _params_ssot(param_names) | |
| param_docs = {p["name"]: p for p in params_ssot} | |
| sig = inspect.signature(func) | |
| relevant_params = [ | |
| p for name, p in param_docs.items() | |
| if name in sig.parameters and p["description"] | |
| ] | |
| if relevant_params: | |
| # Handle functions without docstrings | |
| if func.__doc__ is None: | |
| func.__doc__ = "" | |
| # Simply append SSOT parameters to the existing docstring | |
| ssot_docs = "\n".join(f" {p['name']}: {p['description']}" for p in relevant_params) | |
| func.__doc__ = func.__doc__.rstrip() + "\n" + ssot_docs + "\n" | |
| return func |
🤖 Prompt for AI Agents
In cosmograph/util.py around lines 111 to 136, the decorator assumes
func.__doc__ is non-None and calling .rstrip() will raise on functions without
docstrings; initialize or coalesce the existing docstring to an empty string
before modifying it (e.g. doc = func.__doc__ or "" then build ssot_docs and set
func.__doc__ = doc.rstrip() + "\n" + ssot_docs + "\n"), ensuring you only add
the extra newline when appropriate so functions with no prior docstring get a
valid string and existing docstrings keep their formatting.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/widget-elements.ts(2 hunks)js/widget.css(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/widget-elements.ts
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
| .wrapper .controls .zoomInOutButton div { | ||
| padding: 7px; | ||
| } |
There was a problem hiding this comment.
Zoom buttons lose inner padding in new control containers.
The inner <div> padding still targets only .wrapper .controls, so the top-left/right zoom buttons now render without the 7px padding, shrinking the clickable area. Extend the selector to cover the new containers.
-.wrapper .controls .zoomInOutButton div {
- padding: 7px;
-}
+.wrapper .leftControls .zoomInOutButton div,
+.wrapper .rightControls .zoomInOutButton div,
+.wrapper .controls .zoomInOutButton div {
+ padding: 7px;
+}📝 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.
| .wrapper .controls .zoomInOutButton div { | |
| padding: 7px; | |
| } | |
| .wrapper .leftControls .zoomInOutButton div, | |
| .wrapper .rightControls .zoomInOutButton div, | |
| .wrapper .controls .zoomInOutButton div { | |
| padding: 7px; | |
| } |
🤖 Prompt for AI Agents
In js/widget.css around lines 133 to 135, the rule ".wrapper .controls
.zoomInOutButton div { padding: 7px; }" only targets the old container and
leaves zoom buttons in the new control containers unpadded; update the selector
to also match the new control container classes (for example add the new
container selectors comma-separated such as ".controls-top",
".controls-top-left", ".controls-top-right" or whatever class names are used for
the new containers) so the same 7px padding applies to those zoom button inner
divs, keeping specificity consistent with the existing rule.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
js/cosmograph-data.ts (2)
42-45: Remove duplication when coercing linkTargetBy to an array.Create a small helper/const and reuse it in both places for clarity.
Example:
const toArray = (v?: string | string[]) => (Array.isArray(v) ? v : v != null ? [v] : []); // ... cosmographDataPrepConfig.points.linkTargetsBy = toArray(config.linkTargetBy); // ... linkTargetsBy: toArray(config.linkTargetBy),Also applies to: 50-53
73-82: Consolidate the small-graph simulation overrides.Same predicate repeated 3 times. Merge into a single guarded block.
- // Temporary fix for Cosmograph simulation config parameters for small graphs - if (preparedDataArrow?.points?.numRows !== undefined && preparedDataArrow?.points?.numRows < 50 && config.simulationGravity === undefined) { - config.simulationGravity = 0 - } - if (preparedDataArrow?.points?.numRows !== undefined && preparedDataArrow?.points?.numRows < 50 && config.simulationCenter === undefined) { - config.simulationCenter = 1 - } - if (preparedDataArrow?.points?.numRows !== undefined && preparedDataArrow?.points?.numRows < 50 && config.simulationDecay === undefined) { - config.simulationDecay = 1000 - } + // Temporary fix for Cosmograph simulation config parameters for small graphs + if (preparedDataArrow?.points?.numRows !== undefined && preparedDataArrow.points.numRows < 50) { + if (config.simulationGravity === undefined) config.simulationGravity = 0 + if (config.simulationCenter === undefined) config.simulationCenter = 1 + if (config.simulationDecay === undefined) config.simulationDecay = 1000 + }cosmograph/widget/__init__.py (3)
221-227: get_buffered_arrow_table mutates input DataFrames (int64→int32).The utility assigns converted columns back into the provided df, causing in-place type changes. This can surprise users and break downstream logic.
Prefer making a copy inside utils before assignment:
In cosmograph/widget/utils.py:
def get_buffered_arrow_table(df): if df is None: return None try: df_int32 = df.select_dtypes(include=["int64"]).astype("int32") df_fixed = df.copy() df_fixed[df_int32.columns] = df_int32 table = pa.Table.from_pandas(df_fixed) ...Alternatively (less ideal), pass a copy from here when calling the utility.
335-337: Use logging instead of print; avoid fragile destructor patterns.Replace print with logger; prefer explicit close()/context or weakref.finalize for cleanup instead of relying on del.
Add near top:
import logging logger = logging.getLogger(__name__)Change:
- print(f"Warning: Failed to register instance for API key updates: {e}") + logger.warning("Failed to register instance for API key updates: %s", e)And consider replacing del-based cleanup with a finalize or explicit method.
Also applies to: 344-353
159-159: Traitlets nit: prefer trait instances in List for consistency.Use List(Unicode()) and List(Float()).
- show_labels_for = List(Unicode, default_value=None, allow_none=True).tag(sync=True) + show_labels_for = List(Unicode(), default_value=None, allow_none=True).tag(sync=True) - label_padding = List(Float, default_value=None, allow_none=True).tag(sync=True) + label_padding = List(Float(), default_value=None, allow_none=True).tag(sync=True)Also applies to: 171-171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cosmograph/widget/__init__.py(10 hunks)js/cosmograph-data.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cosmograph/widget/__init__.py (3)
cosmograph/widget/utils.py (1)
get_buffered_arrow_table(17-36)cosmograph/widget/export_project/__init__.py (1)
export_project(14-92)cosmograph/config.py (3)
register_instance(33-40)unregister_instance(43-51)get_api_key(23-30)
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (3)
cosmograph/widget/__init__.py (2)
248-260: Polygonal selection + validation: LGTM.Good API shape and basic input validation.
319-327: export_project_by_name: confirmed return type Returns the project ID string; update docs and usage examples to reflect this.js/cosmograph-data.ts (1)
5-5: Code changes verified; cross-config mapping is complete and correct.Config-props mapping confirmed:
link_target_by→linkTargetByinjs/config-props.json(line 84). Python-side type definition (Union[Unicode(), List(Unicode())]incosmograph/widget/__init__.pyline 144) supports both string and array variants. JavaScript coercion logic injs/cosmograph-data.ts(lines 42–45, 50–53) correctly normalizesstring | string[]tostring[]for both points and links blocks. Export functionality (cosmograph/widget/export_project/create_project.pylines 149–152) correctly useslinkTargetBy. End-to-end integration is consistent.
| clicked_cluster = Any(None, allow_none=True).tag(sync=True) | ||
| selected_point_indices = List(Int, allow_none=True).tag(sync=True) | ||
| selected_point_ids = List(Unicode, allow_none=True).tag(sync=True) | ||
| cosmograph_config = Dict(default_value={}, allow_none=True).tag(sync=True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid mutable default for cosmograph_config (risk of cross‑instance leaks).
Defaulting Dict to {} shares the same object across instances. Use None and initialize in init.
- cosmograph_config = Dict(default_value={}, allow_none=True).tag(sync=True)
+ cosmograph_config = Dict(default_value=None, allow_none=True).tag(sync=True)Initialize safely during construction:
if self.api_key is None:
global_api_key = get_api_key()
if global_api_key is not None:
self.api_key = global_api_key
+
+ # Ensure per-instance dict (avoid shared mutable default)
+ if self.cosmograph_config is None:
+ self.cosmograph_config = {}Also applies to: 338-343
🤖 Prompt for AI Agents
In cosmograph/widget/__init__.py around line 214 (and also apply the same change
to the similar block at lines 338-343), the trait Dict is initialized with a
mutable default_value {} which can be shared across instances; change the trait
to use default_value=None (keeping allow_none=True) and then in the class
__init__ detect if cosmograph_config is None and assign a new empty dict (or a
shallow copy) to self.cosmograph_config so each instance gets its own dict;
update both locations accordingly to avoid cross-instance state leakage.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
js/widget.css (1)
130-132: Extend zoom button inner padding to new containers.-.wrapper .controls .zoomInOutButton div { - padding: 7px; -} +.wrapper .leftControls .zoomInOutButton div, +.wrapper .rightControls .zoomInOutButton div, +.wrapper .controls .zoomInOutButton div { + padding: 7px; +}js/widget.ts (1)
66-68: Typo breaks unselect; call the correct API.- if (msg.type === 'unselect_points_by_indices') { - cosmograph?.unselectPointsByIndicies(msg.indices) - } + if (msg.type === 'unselect_points_by_indices') { + cosmograph?.unselectPointsByIndices(msg.indices) + }
🧹 Nitpick comments (8)
js/widget-elements.ts (2)
47-60: Clamp RGBA inputs to valid ranges to avoid invalid CSS colors.When color arrays come from user input, values may exceed [0..1]. Clamp r,g,b to [0..255] after scaling and a to [0..1] for robustness.
} else if (Array.isArray(color) && color.length >= 3) { // Convert RGB array to CSS color - const r = Math.round(color[0] * 255) - const g = Math.round(color[1] * 255) - const b = Math.round(color[2] * 255) - const a = color.length > 3 ? color[3] : 1 + const clamp = (v:number,min:number,max:number)=>Math.min(max,Math.max(min,v)) + const r = clamp(Math.round(color[0] * 255), 0, 255) + const g = clamp(Math.round(color[1] * 255), 0, 255) + const b = clamp(Math.round(color[2] * 255), 0, 255) + const a = clamp(color.length > 3 ? Number(color[3]) : 1, 0, 1) loadingOverlay.style.backgroundColor = `rgba(${r}, ${g}, ${b}, ${a})` }
22-33: A11y: add ARIA roles and avoid hard-coded timeout coupling.
- Add role="status" aria-live and aria-busy to overlay for screen readers.
- Prefer transitionend to avoid drift if the CSS duration changes.
export function createLoadingOverlay(el: HTMLElement): HTMLDivElement { const loadingOverlay = document.createElement('div') loadingOverlay.classList.add('loading-overlay') + loadingOverlay.setAttribute('role', 'status') + loadingOverlay.setAttribute('aria-live', 'polite') + loadingOverlay.setAttribute('aria-busy', 'true') const spinner = document.createElement('div') spinner.classList.add('loading-spinner') loadingOverlay.appendChild(spinner) el.appendChild(loadingOverlay) return loadingOverlay } export function hideLoadingOverlay(loadingOverlay: HTMLDivElement): void { loadingOverlay.style.opacity = '0' - setTimeout(() => { - loadingOverlay.style.display = 'none' - }, 300) // Match the CSS transition duration + const onEnd = () => { + loadingOverlay.style.display = 'none' + loadingOverlay.removeEventListener('transitionend', onEnd) + loadingOverlay.setAttribute('aria-busy', 'false') + } + loadingOverlay.addEventListener('transitionend', onEnd) }Also applies to: 35-45
js/components/point-info.ts (2)
58-61: Avoid innerHTML; use safer DOM APIs.No user input here, but prefer textContent/replaceChildren to eliminate XSS surface and satisfy linters.
- this.panel.innerHTML = '' + this.panel.replaceChildren() - const closeButton = document.createElement('button') + const closeButton = document.createElement('button') closeButton.className = 'point-info-close' - closeButton.innerHTML = '×' + closeButton.textContent = '×' closeButton.onclick = () => this.hide() this.panel.appendChild(closeButton)Also applies to: 123-129
29-56: Arrow table extraction: guard optional APIs.If getPointsByIndices returns nullish or schemas without fields, current code handles first case but not malformed schema. Add narrow check before mapping fields to reduce runtime errors on edge builds.
- const columnNames = pointData.schema.fields.map((field: any) => field.name) + const columnNames = Array.isArray(pointData?.schema?.fields) + ? pointData.schema.fields.map((field: any) => field.name) + : []js/components/settings-panel.ts (2)
43-50: Replace innerHTML and add accessible label for the settings toggle.Use textContent and aria-label; keeps UI identical and resolves XSS/lint warnings.
- const toggleButton = document.createElement('button') + const toggleButton = document.createElement('button') toggleButton.className = 'settings-panel-toggle' - toggleButton.innerHTML = '⚙️' + toggleButton.textContent = '⚙️' toggleButton.title = 'Settings' + toggleButton.setAttribute('aria-label', 'Settings')
139-184: Type-safety nit: mixed Map values.You store both inputs and spans in a Map<string, HTMLInputElement> and cast via any. Consider two maps or a discriminated union to drop the casts. Optional.
js/widget.css (1)
1-7: Contain absolutely positioned children within the widget.Add position: relative to .wrapper so .bottom, .leftControls, .rightControls, and .loading-overlay position correctly within the widget bounds.
.wrapper { overflow: auto; height: 450px; display: flex; flex-flow: column; color: #ccc; + position: relative; }Also applies to: 144-160
js/widget.ts (1)
20-27: Optional: react to background_color changes for overlay color consistency.If background_color changes before data loads, consider subscribing to it to update the overlay color live.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
js/components/control-buttons.ts(2 hunks)js/components/point-info.ts(1 hunks)js/components/settings-panel.ts(1 hunks)js/widget-elements.ts(2 hunks)js/widget.css(2 hunks)js/widget.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
js/widget.ts (8)
js/widget-elements.ts (5)
createWidgetContainer(1-20)createLoadingOverlay(22-33)showLoadingOverlay(35-38)setLoadingOverlayBackgroundColor(47-60)hideLoadingOverlay(40-45)js/components/point-timeline.ts (1)
PointTimeline(4-43)js/components/link-timeline.ts (1)
LinkTimeline(4-44)js/components/settings-panel.ts (1)
SettingsPanel(10-355)js/components/point-info.ts (1)
PointInfoPanel(3-144)js/helper.ts (1)
subscribe(11-14)js/cosmograph-data.ts (1)
prepareCosmographDataAndMutate(12-90)js/components/control-buttons.ts (1)
ControlButtonsComponent(4-23)
js/components/settings-panel.ts (1)
cosmograph/widget/__init__.py (1)
Cosmograph(26-354)
js/components/control-buttons.ts (1)
js/widget-elements.ts (1)
createWidgetControlElements(106-144)
🪛 ast-grep (0.39.6)
js/components/point-info.ts
[warning] 125-125: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: closeButton.innerHTML = '×'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 58-58: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: this.panel.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 125-125: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: closeButton.innerHTML = '×'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
js/components/settings-panel.ts
[warning] 45-45: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: toggleButton.innerHTML = '⚙️'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 45-45: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: toggleButton.innerHTML = '⚙️'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🪛 Biome (2.1.2)
js/widget.css
[error] 374-374: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
display is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (2)
js/widget-elements.ts (1)
106-143: Controls DOM restructure looks good.Return shape preserved; new settings container wired. No issues spotted.
js/components/control-buttons.ts (1)
1-1: Validate runtime availability of polygonal button in targeted Cosmograph version.Switch to CosmographButtonPolygonalSelection looks correct, but confirm the package version in this PR exposes it and the default options object is acceptable.
Would you like me to scan package.json/lock to confirm the @cosmograph/cosmograph version and exported symbols?
Also applies to: 10-12, 16-22
| constructor( | ||
| cosmograph: Cosmograph, | ||
| panelContainer: HTMLElement, | ||
| buttonContainer: HTMLElement, | ||
| model: RenderProps['model'] | ||
| ) { | ||
| this.panelContainer = panelContainer | ||
| this.buttonContainer = buttonContainer | ||
| this.model = model | ||
| this.cosmograph = cosmograph | ||
| this.panel = this.createPanel() | ||
| this.panelContainer.appendChild(this.panel) | ||
| this.setupModelListeners() | ||
| this.syncFromCosmograph() | ||
| this.updateSimulationVisibility() | ||
| this.updateLabelsCheckboxState() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add proper teardown: unsubscribe model listeners and remove the toggle button.
Listeners are added but never removed; also the toggle button remains after destroy(). Track unsubscribers and clean them up.
+import { subscribe } from '../helper'
@@
export class SettingsPanel {
@@
private controls: Map<string, HTMLInputElement> = new Map()
private simulationSection: HTMLDivElement | null = null
private showLabelsContainer: HTMLDivElement | null = null
+ private unsubscribers: Array<() => void> = []
+ private toggleButtonEl: HTMLButtonElement | null = null
@@
- const toggleButton = document.createElement('button')
+ const toggleButton = document.createElement('button')
toggleButton.className = 'settings-panel-toggle'
toggleButton.textContent = '⚙️'
toggleButton.title = 'Settings'
toggleButton.onclick = () => this.togglePanel()
this.buttonContainer.appendChild(toggleButton)
+ this.toggleButtonEl = toggleButton
@@
- properties.forEach((snakeProp) => {
- this.model.on(`change:${snakeProp}`, () => {
+ properties.forEach((snakeProp) => {
+ const handler = () => {
// Update simulation visibility if embedding properties change
@@
- })
+ }
+ this.unsubscribers.push(subscribe(this.model as any, `change:${snakeProp}`, handler))
})
@@
public destroy(): void {
- this.panel.remove()
+ this.unsubscribers.forEach(unsub => unsub())
+ this.unsubscribers = []
+ this.panel.remove()
+ if (this.toggleButtonEl) {
+ this.toggleButtonEl.remove()
+ this.toggleButtonEl = null
+ }
}Also applies to: 296-345, 352-354
🤖 Prompt for AI Agents
In js/components/settings-panel.ts around lines 21 to 37 (also apply same fix at
296-345 and 352-354), the constructor registers model/cosmograph listeners and
creates a toggle button but destroy/teardown does not remove them; update the
class to track and store unsubscribe functions (or listener references) when
calling setupModelListeners and any cosmograph subscriptions, push them into an
array like this.unsubscribers, and in destroy() iterate and call each
unsubscriber, then remove the toggle button and the panel from the DOM, and null
out references (panel, button, model, cosmograph, containers) to avoid leaks;
ensure removeEventListener is used if raw DOM listeners were attached and that
destroy is idempotent (guard against double teardown).
| /* Point Info Panel */ | ||
| .point-info-panel { | ||
| position: absolute; | ||
| top: 0.5rem; | ||
| left: 3rem; | ||
| z-index: 100; | ||
| pointer-events: auto; | ||
| display: none; | ||
| width: 240px; | ||
| max-height: calc(100% - 1rem); | ||
| background: rgba(45, 49, 58, 0.95); | ||
| backdrop-filter: blur(10px); | ||
| border: 1px solid rgba(255, 255, 255, 0.1); | ||
| border-radius: 8px; | ||
| box-shadow: 0 8px 24px rgba(0, 0, 0, 0.4); | ||
| overflow: hidden; | ||
| display: flex; | ||
| flex-direction: column; | ||
| } |
There was a problem hiding this comment.
Fix duplicate display property; panel might render unexpectedly.
Keep it hidden by default and let JS toggle to flex.
.point-info-panel {
@@
- display: none;
+ display: none;
@@
- display: flex;
+ /* display is controlled via JS (flex when visible) */📝 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.
| /* Point Info Panel */ | |
| .point-info-panel { | |
| position: absolute; | |
| top: 0.5rem; | |
| left: 3rem; | |
| z-index: 100; | |
| pointer-events: auto; | |
| display: none; | |
| width: 240px; | |
| max-height: calc(100% - 1rem); | |
| background: rgba(45, 49, 58, 0.95); | |
| backdrop-filter: blur(10px); | |
| border: 1px solid rgba(255, 255, 255, 0.1); | |
| border-radius: 8px; | |
| box-shadow: 0 8px 24px rgba(0, 0, 0, 0.4); | |
| overflow: hidden; | |
| display: flex; | |
| flex-direction: column; | |
| } | |
| /* Point Info Panel */ | |
| .point-info-panel { | |
| position: absolute; | |
| top: 0.5rem; | |
| left: 3rem; | |
| z-index: 100; | |
| pointer-events: auto; | |
| display: none; | |
| width: 240px; | |
| max-height: calc(100% - 1rem); | |
| background: rgba(45, 49, 58, 0.95); | |
| backdrop-filter: blur(10px); | |
| border: 1px solid rgba(255, 255, 255, 0.1); | |
| border-radius: 8px; | |
| box-shadow: 0 8px 24px rgba(0, 0, 0, 0.4); | |
| overflow: hidden; | |
| /* display is controlled via JS (flex when visible) */ | |
| flex-direction: column; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 374-374: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
display is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
js/widget.css around lines 358 to 376, the .point-info-panel rules declare
display twice (display: none; and later display: flex;) which can cause
unexpected rendering; remove the second display: flex so the panel remains
hidden by default and preserve the flex-related properties (flex-direction,
overflow, etc.), and update the JS that toggles the panel to add/remove a class
or set display to flex when showing the panel.
| const modelChangeHandlers: { [key: string]: () => void } = { | ||
| _ipc_points: () => { | ||
| const ipc = model.get('_ipc_points') | ||
| cosmographConfig.points = ipc ? tableFromIPC(ipc.buffer) : undefined | ||
| cosmographConfig.points = ipc ? ipc.buffer : undefined | ||
| }, | ||
| _ipc_links: () => { | ||
| const ipc = model.get('_ipc_links') | ||
| cosmographConfig.links = ipc ? tableFromIPC(ipc.buffer) : undefined | ||
| cosmographConfig.links = ipc ? ipc.buffer : undefined | ||
| }, | ||
|
|
||
| disable_point_size_legend: async () => { | ||
| await legends.updateLegend('point', 'size') | ||
| disable_point_size_legend: () => { | ||
| legends.update('point', 'size') | ||
| }, | ||
| disable_link_width_legend: async () => { | ||
| await legends.updateLegend('link', 'width') | ||
| disable_link_width_legend: () => { | ||
| legends.update('link', 'width') | ||
| }, | ||
| disable_point_color_legend: async () => { | ||
| await legends.updateLegend('point', 'color') | ||
| disable_point_color_legend: () => { | ||
| legends.update('point', 'color') | ||
| }, | ||
| disable_link_color_legend: async () => { | ||
| await legends.updateLegend('link', 'color') | ||
| disable_link_color_legend: () => { | ||
| legends.update('link', 'color') | ||
| }, | ||
| } |
There was a problem hiding this comment.
Data updates for points/links don’t reach Cosmograph.
Handlers for _ipc_points/_ipc_links update cosmographConfig but never call setConfig, so live data changes won’t render.
const modelChangeHandlers: { [key: string]: () => void } = {
_ipc_points: () => {
const ipc = model.get('_ipc_points')
cosmographConfig.points = ipc ? ipc.buffer : undefined
},
_ipc_links: () => {
const ipc = model.get('_ipc_links')
cosmographConfig.links = ipc ? ipc.buffer : undefined
},
@@
- .map(([snakeCaseProp, onModelChange]) => subscribe(model, `change:${snakeCaseProp}`, async () => {
+ .map(([snakeCaseProp, onModelChange]) => subscribe(model, `change:${snakeCaseProp}`, async () => {
onModelChange()
- if (snakeToCamelConfigProps[snakeCaseProp as keyof typeof snakeToCamelConfigProps]) {
+ const isConfigProp =
+ !!snakeToCamelConfigProps[snakeCaseProp as keyof typeof snakeToCamelConfigProps]
+ const isDataProp = snakeCaseProp === '_ipc_points' || snakeCaseProp === '_ipc_links'
+
+ if (isConfigProp || isDataProp) {
cosmograph?.setConfig(cosmographConfig)
// await when config is set
await cosmograph?.getConfig()
updatePythonCosmographConfig()
}Also applies to: 174-186
🤖 Prompt for AI Agents
In js/widget.ts around lines 137 to 159 (and similarly for lines 174 to 186),
the _ipc_points and _ipc_links handlers only update cosmographConfig but do not
call setConfig, so the Cosmograph instance never receives the updated data;
modify those handlers to call setConfig after assigning cosmographConfig.points
or cosmographConfig.links (ensuring you pass the updated cosmographConfig or the
specific updated fields depending on setConfig signature) so the live data
changes are applied and rendered.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cosmograph/base.py (1)
451-457: Guard against non-trait kwargs leaking to widget (changePoints/changeLinks).cosmo_advanced accepts changePoints/changeLinks but widget has no such traits. If provided, these would raise on Cosmograph(**kwargs). Drop them before validation/instantiation.
@@ - kwargs = locals().copy() + kwargs = locals().copy() + # Remove Python-only callables that are not widget traits + kwargs.pop("changePoints", None) + kwargs.pop("changeLinks", None)Also applies to: 483-491, 498-499
♻️ Duplicate comments (1)
cosmograph/widget/__init__.py (1)
216-216: Avoid shared mutable default for cosmograph_config; initialize per-instance.Defaulting Dict to {} shares state across instances. Use None + set in init.
- cosmograph_config = Dict(default_value={}, allow_none=True).tag(sync=True) + cosmograph_config = Dict(default_value=None, allow_none=True).tag(sync=True) @@ def __init__(self, **kwargs): super().__init__(**kwargs) @@ if self.api_key is None: global_api_key = get_api_key() if global_api_key is not None: self.api_key = global_api_key + + # Ensure per-instance mutable config + if self.cosmograph_config is None: + self.cosmograph_config = {}Based on learnings
Also applies to: 331-345
🧹 Nitpick comments (4)
cosmograph/base.py (1)
78-79: Broaden type for link_target_by to match widget (list allowed).Widget supports Union[str, list[str]]; cosmo narrows to str. Widen to avoid misleading users and for doc parity.
- link_target_by: str = None, + link_target_by: Union[str, list[str]] = None,cosmograph/widget/__init__.py (3)
334-339: Use warnings/logging instead of print for registration failure.Avoid stdout noise in notebooks; prefer warnings.warn.
- except Exception as e: - # Log warning but don't fail initialization - print(f"Warning: Failed to register instance for API key updates: {e}") + except Exception as e: + import warnings + warnings.warn(f"Failed to register instance for API key updates: {e}")
321-329: Clarify and standardize return value of export_project_by_name.export_project returns a project_id (string), but earlier docs say dict. Return a small dict with id+URL for convenience, or update docs accordingly.
- try: - return export_project(self.api_key, project_name, self.points, self.links, self.cosmograph_config) + try: + project_id = export_project(self.api_key, project_name, self.points, self.links, self.cosmograph_config) + return { + "project_id": project_id, + "project_url": f"https://run.cosmograph.app/project/{project_id}", + }
221-229: Ensure IPC conversion doesn’t mutate user DataFrames.get_buffered_arrow_table currently casts int64→int32 in place (per utils snippet). Copy before casting to avoid side effects on user data.
Would you like a PR to update cosmograph/widget/utils.py:get_buffered_arrow_table to operate on a df.copy() and assign into the copy instead? Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cosmograph/base.py(11 hunks)cosmograph/widget/__init__.py(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cosmograph/base.py (3)
cosmograph/util.py (1)
add_cosmo_param_descriptions(111-135)cosmograph/config.py (1)
get_api_key(23-30)cosmograph/widget/__init__.py (1)
Cosmograph(26-355)
cosmograph/widget/__init__.py (3)
cosmograph/widget/utils.py (1)
get_buffered_arrow_table(17-36)cosmograph/widget/export_project/__init__.py (1)
export_project(14-92)cosmograph/config.py (3)
register_instance(33-40)unregister_instance(43-51)get_api_key(23-30)
⏰ 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: Windows Validation (3.10)
🔇 Additional comments (1)
cosmograph/base.py (1)
179-187: Now I need to directly examine the codebase to understand the signature building. Let me run a script to check the implementation:Based on my analysis of the script output, I now have enough information to verify the concern. The output clearly shows:
- Widget exposes
api_key:api_key = Unicode(None, allow_none=True)exists at cosmograph/widget/init.py:218- SSOT includes
api_key: params_ssot.json lines 717-720 defines api_key as a parameter with proper metadata- Signature building:
cosmograph_base_signature()is called at cosmograph/base.py:19 to createcosmo_base_sig- Validation uses the signature:
validate_kwargs()at line 240 usesset(cosmo_base_sig.names)The critical question is: Does
cosmograph_base_signature()properly includeapi_keyfrom the SSOT?Looking at the output, the search results show that:
_params_ssot()is used to fetch parameters from the SSOTapi_keyis defined in the params_ssot.json file- The signature is created from these parameters
Since
api_keyis:
- ✅ Defined in params_ssot.json (lines 717-720)
- ✅ Exposed in the widget (cosmograph/widget/init.py:218)
- ✅ Listed as a parameter in function signatures (cosmograph/base.py:155, 403)
- ✅ Included in the global usage (line 581, 587 - argument_aliases generation uses
cosmo_base_sig.names)The validation logic should work correctly because
api_keyWILL be incosmo_base_sig.namessince it comes from the SSOT which is the single source of truth. The injection logic at lines 182-186 (and 492-496) safely adds the global api_key after validation passes.Verdict: The review concern is unfounded. The code properly validates kwargs first, and
api_keyis included in the valid names from the signature, so user-suppliedapi_keywill NOT be rejected.Rewritten Review Comment:
api_key is correctly included in validation; no action required.
The
api_keyparameter is defined in the SSOT (params_ssot.json), properly exposed by the widget trait, and included incosmo_base_sig.namesused byvalidate_kwargs(). User-supplied api_key values will pass validation before the injection logic (lines 182-186, 492-496) applies the global fallback. The implementation is correct as-is.
| show_fps_monitor: bool = None, | ||
| pixel_ratio: float = None, | ||
| # Set to True by default until it becomes the default in Cosmograph library | ||
| scale_points_on_zoom: bool = True, |
There was a problem hiding this comment.
Fix param name mismatch: normalize show_fps_monitor → show_FPS_monitor before constructing widget.
Widget exposes show_FPS_monitor, but cosmo accepts show_fps_monitor and forwards it verbatim, causing an unknown trait error at Cosmograph(**kwargs).
Apply this minimal aliasing before validation (same fix also recommended in cosmo_advanced):
@@
- # Remove None values
- kwargs = remove_none_values(kwargs)
+ # Normalize aliases that differ only by capitalization/acronyms
+ if "show_fps_monitor" in kwargs and "show_FPS_monitor" not in kwargs:
+ kwargs["show_FPS_monitor"] = kwargs.pop("show_fps_monitor")
+
+ # Remove None values
+ kwargs = remove_none_values(kwargs)Also applies to: 188-189
🤖 Prompt for AI Agents
In cosmograph/base.py around lines 113-116 (and also apply the same change at
lines 188-189), the function accepts show_fps_monitor but the widget expects
show_FPS_monitor, causing an unknown trait error when passing kwargs through;
before constructing/validating the widget, detect if kwargs contains
show_fps_monitor and if so move/alias its value to show_FPS_monitor (and remove
or leave the original key out of the forwarded kwargs) so the correct trait name
is passed into Cosmograph(**kwargs); apply the same aliasing in the
cosmo_advanced location mentioned.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
demos/colab_Mobius_in_Cosmograph_Widget.ipynb (2)
43-52: Add context and guidance for API key usage.The API key cell lacks documentation about where users can obtain their API key, how to securely manage it (e.g., using environment variables), and what to expect if authentication fails. Consider adding a markdown cell before this code cell with:
- Instructions on obtaining an API key from Cosmograph
- Best practices for secure key management (e.g.,
set_api_key(os.getenv('COSMOGRAPH_API_KEY')))- Link to relevant documentation
Example markdown cell to add:
## API Key Configuration To use the export feature, you'll need a Cosmograph API key: 1. Obtain your API key from [Cosmograph platform](https://cosmograph.app/settings/api) 2. For security, store it as an environment variable or use a secrets manager 3. Never commit API keys to version control ```python # Recommended: Use environment variables import os set_api_key(os.getenv('COSMOGRAPH_API_KEY')) # For demo purposes only: set_api_key("Your API key here")--- `406-435`: **Consider adding error handling and usage guidance.** The export cell doesn't demonstrate error handling for common failure scenarios (invalid API key, network issues, etc.) or explain what users should do with the returned project ID. Consider adding: - A try-except block showing graceful error handling - A markdown cell explaining the export process and next steps - Information about accessing the exported project Example enhancement: ```python try: project_id = graph.export_project_by_name("Test Export of Mobius example") print(f"✅ Export successful! View your project at: https://run.cosmograph.app/project/{project_id}") except Exception as e: print(f"❌ Export failed: {e}") print("Please check your API key and network connection")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
demos/colab_Mobius_in_Cosmograph_Widget.ipynb(5 hunks)
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (2)
demos/colab_Mobius_in_Cosmograph_Widget.ipynb (2)
39-39: LGTM!The addition of
set_api_keyto the import statement is appropriate for demonstrating the new API key configuration feature.
406-435: I need to check how the Möbius visualization was created in the notebook to understand if points were explicitly passed.Based on my verification, I can now provide a conclusive assessment.
The "0 points" output is expected behavior.
Cosmograph's graph mode can infer nodes from a links edge list containing source and target columns, which allows visualization with links-only data. However, the standard pattern shown in documentation provides explicit points DataFrames to the
cosmo()function.The notebook provides only a
linksDataFrame tocosmo(), which allows the Möbius strip to visualize (nodes are inferred from link sources/targets), but since no explicitpointsDataFrame was passed,self.pointsremainsNoneor empty. The export function correctly passesself.pointsto the API, but there's nothing to export.
⚠️ Note: Vibe coding only
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
cosmograph/widget/__init__.py (1)
217-217: Mutable default for cosmograph_config still needs addressing.This issue was previously flagged: using
{}asdefault_valuefor a Dict trait can lead to shared state across instances in certain traitlets versions. Although modern traitlets typically handles this correctly by creating new instances, the safer pattern is to usedefault_value=Noneand initialize in__init__.As suggested in the previous review, apply this pattern:
- cosmograph_config = Dict(default_value={}, allow_none=True).tag(sync=True) + cosmograph_config = Dict(default_value=None, allow_none=True).tag(sync=True)And in
__init__(after line 345):if self.api_key is None: global_api_key = get_api_key() if global_api_key is not None: self.api_key = global_api_key + + # Ensure per-instance dict (avoid shared mutable default) + if self.cosmograph_config is None: + self.cosmograph_config = {}Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cosmograph/__about__.py(1 hunks)cosmograph/widget/__init__.py(10 hunks)cosmograph/widget/export_project/__init__.py(1 hunks)cosmograph/widget/export_project/create_project.py(1 hunks)cosmograph/widget/export_project/upload_file.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cosmograph/about.py
- cosmograph/widget/export_project/init.py
🧰 Additional context used
🧬 Code graph analysis (1)
cosmograph/widget/__init__.py (3)
cosmograph/widget/utils.py (1)
get_buffered_arrow_table(17-36)cosmograph/widget/export_project/__init__.py (1)
export_project(70-191)cosmograph/config.py (3)
register_instance(33-40)unregister_instance(43-51)get_api_key(23-30)
🔇 Additional comments (11)
cosmograph/widget/__init__.py (10)
6-10: LGTM! Imports are well-organized.The new imports for Dict trait, utility functions, export functionality, and API-key lifecycle are all appropriate for the expanded feature set.
121-123: Dict trait definition is correct.The
point_color_by_maptrait properly usesDictwith explicitkey_traitandvalue_traitspecifications, allowing string keys mapped to either color strings or RGB float arrays.
135-137: Dict trait for cluster positions is well-defined.The
cluster_positions_mapcorrectly specifies a Dict mapping cluster identifiers (Unicode) to coordinate pairs (List of Float).
221-229: Good refactoring to use shared utility.Delegating Arrow IPC serialization to the external
get_buffered_arrow_tableutility improves code reuse and maintainability. The utility properly handles None values and error cases.
251-267: Polygonal selection methods are well-implemented.The methods properly validate input (especially
select_points_in_polygonwhich checks polygon structure) and delegate to the frontend via message passing. Good defensive programming.
322-330: Export method is robust and well-validated.The implementation includes comprehensive validation (API key and project name), proper error handling with context preservation, and returns the export result. The addition of the
debugparameter for detailed logging is a good improvement.
332-346: Initialization logic is well-designed.The
__init__method properly:
- Calls parent constructor
- Gracefully handles registration failures without breaking initialization
- Provides sensible fallback to global API key
This defensive approach ensures the widget remains functional even if API-key infrastructure has issues.
347-356: Cleanup logic follows best practices.The
__del__method properly handles cleanup:
- Suppresses exceptions during unregistration (appropriate for finalizers)
- Safely checks for parent
__del__existence before callingThis defensive approach prevents cleanup failures from propagating.
91-93: Explicit default with clear rationale.Setting
scale_points_on_zoom = Trueas a temporary default until the upstream library changes is appropriate, and the comment clearly documents the reasoning.
175-177: Explicit default improves user experience.Defaulting
show_hovered_point_labelto True provides better initial UX, and the comment documents this is temporary pending upstream changes.cosmograph/widget/export_project/create_project.py (1)
191-199: Excellent security practice masking API keys.The debug logging properly masks the API key before logging configuration details, preventing credential leakage in logs. This is a security best practice.
| def create_empty_project( | ||
| api_key: str, | ||
| project_name: str, | ||
| description: str = "Project created via API", | ||
| debug: bool = False, | ||
| ) -> dict[str, Any]: | ||
| """Create an empty Cosmograph project. | ||
| Args: | ||
| api_key: Cosmograph API key | ||
| project_name: Name for the project | ||
| description: Description for the project | ||
| Returns: | ||
| dict: API response from project creation containing project ID | ||
| Raises: | ||
| ValueError: If project creation fails | ||
| """ | ||
| try: | ||
| config_json = { | ||
| "json": { | ||
| "apiKey": api_key, | ||
| "config": { | ||
| "title": project_name, | ||
| "description": description, | ||
| }, | ||
| }, | ||
| } | ||
| # logger.info("Creating empty project with config: %s", json.dumps(config_json, indent=4)) | ||
| start_time = time.perf_counter() if debug else None | ||
| response = requests.post( | ||
| f"{API_BASE}/publicApi.createProject", | ||
| json=config_json, | ||
| ) | ||
| response.raise_for_status() | ||
| if debug: | ||
| logger.info("⏱️ - createProject API request took: %.3f seconds", time.perf_counter() - start_time) | ||
|
|
||
| result = response.json() | ||
| # logger.info("✅ Empty project created successfully: %s", json.dumps(result, indent=4)) | ||
| return result | ||
| except requests.RequestException as e: | ||
| logger.error("❌ Failed to create empty project '%s': %s", project_name, e) | ||
| msg = f"Failed to create empty project: {e}" | ||
| raise ValueError(msg) from e |
There was a problem hiding this comment.
Add timeout and input validation.
The create_empty_project function has two concerns:
- Missing timeout: The POST request lacks a timeout, risking indefinite hangs
- No input validation: Empty
api_keyorproject_nameare not validated
Apply these changes:
def create_empty_project(
api_key: str,
project_name: str,
description: str = "Project created via API",
debug: bool = False,
) -> dict[str, Any]:
"""Create an empty Cosmograph project.
Args:
api_key: Cosmograph API key
project_name: Name for the project
description: Description for the project
Returns:
dict: API response from project creation containing project ID
Raises:
ValueError: If project creation fails
"""
+ if not api_key or not api_key.strip():
+ raise ValueError("API key cannot be empty")
+ if not project_name or not project_name.strip():
+ raise ValueError("Project name cannot be empty")
+
try:
config_json = {
"json": {
"apiKey": api_key,
"config": {
"title": project_name,
"description": description,
},
},
}
# logger.info("Creating empty project with config: %s", json.dumps(config_json, indent=4))
start_time = time.perf_counter() if debug else None
response = requests.post(
f"{API_BASE}/publicApi.createProject",
json=config_json,
+ timeout=30,
)Based on learnings
🤖 Prompt for AI Agents
In cosmograph/widget/export_project/create_project.py around lines 11-56, the
function lacks input validation and a request timeout: validate that api_key and
project_name are non-empty (strip whitespace and raise ValueError with a clear
message if invalid) before making the request, and add a sensible timeout to
requests.post (e.g., timeout=10 or a new optional timeout parameter with a
default) so the call cannot hang indefinitely; update logging/error messages
accordingly and ensure existing except blocks still surface
timeout/RequestException errors as ValueError.
| def create_project( | ||
| api_key: str, | ||
| project_name: str, | ||
| points_data: Optional[dict[str, Any]] = None, | ||
| links_data: Optional[dict[str, Any]] = None, | ||
| cosmograph_config: Optional[dict[str, Any]] = None, | ||
| debug: bool = False, | ||
| ) -> dict[str, Any]: | ||
| """Create a new Cosmograph project with the uploaded files. | ||
| Raises: | ||
| ValueError: If project creation fails | ||
| Args: | ||
| api_key: Cosmograph API key | ||
| project_name: Name for the project | ||
| points_data: Data for points table | ||
| links_data: Data for links table | ||
| cosmograph_config: Cosmograph config | ||
| Returns: | ||
| dict: API response from project creation | ||
| """ | ||
|
|
||
| cosmograph_config = cosmograph_config or {} | ||
| data_sources = [] | ||
| column_mapping = { | ||
| "points": {"columns": {}}, | ||
| "links": {"columns": {}}, | ||
| } | ||
|
|
||
| # Configure points data source and column mapping | ||
| if points_data: | ||
| data_sources.append({ | ||
| "type": "file", | ||
| "tableName": "point_table", | ||
| "fileName": points_data["file_name"], | ||
| }) | ||
|
|
||
| # Map points columns according to API specifications | ||
| points_columns = {} | ||
|
|
||
| # Required columns | ||
| point_id_by = cosmograph_config.get("pointIdBy") | ||
| if point_id_by: | ||
| points_columns["id"] = point_id_by | ||
|
|
||
| # Optional columns - map only if present in config | ||
| point_label_by = cosmograph_config.get("pointLabelBy") | ||
| if point_label_by: | ||
| points_columns["label"] = point_label_by | ||
|
|
||
| point_color_by = cosmograph_config.get("pointColorBy") | ||
| if point_color_by: | ||
| points_columns["color"] = point_color_by | ||
|
|
||
| point_size_by = cosmograph_config.get("pointSizeBy") | ||
| if point_size_by: | ||
| points_columns["size"] = point_size_by | ||
|
|
||
| point_x_by = cosmograph_config.get("pointXBy") | ||
| if point_x_by: | ||
| points_columns["x"] = point_x_by | ||
|
|
||
| point_y_by = cosmograph_config.get("pointYBy") | ||
| if point_y_by: | ||
| points_columns["y"] = point_y_by | ||
|
|
||
| point_cluster_by = cosmograph_config.get("pointClusterBy") | ||
| if point_cluster_by: | ||
| points_columns["cluster"] = point_cluster_by | ||
|
|
||
| point_timeline_by = cosmograph_config.get("pointTimelineBy") | ||
| if point_timeline_by: | ||
| points_columns["time"] = point_timeline_by | ||
|
|
||
| if points_columns: | ||
| column_mapping["points"] = { | ||
| "columns": points_columns, | ||
| "tableName": "point_table", | ||
| } | ||
|
|
||
| # Configure links data source and column mapping | ||
| if links_data: | ||
| data_sources.append({ | ||
| "type": "file", | ||
| "tableName": "link_table", | ||
| "fileName": links_data["file_name"], | ||
| }) | ||
|
|
||
| # Map links columns according to API specifications | ||
| links_columns = {} | ||
|
|
||
| # Required columns | ||
| link_source_by = cosmograph_config.get("linkSourceBy") | ||
| link_target_by = cosmograph_config.get("linkTargetBy") | ||
| if link_source_by and link_target_by: | ||
| links_columns["source"] = link_source_by | ||
| links_columns["target"] = link_target_by | ||
|
|
||
| # Optional columns - map only if present in config | ||
| link_width_by = cosmograph_config.get("linkWidthBy") | ||
| if link_width_by: | ||
| links_columns["width"] = link_width_by | ||
|
|
||
| link_color_by = cosmograph_config.get("linkColorBy") | ||
| if link_color_by: | ||
| links_columns["color"] = link_color_by | ||
|
|
||
| # Link timeline support | ||
| link_timeline_by = cosmograph_config.get("linkTimelineBy") | ||
| if link_timeline_by: | ||
| links_columns["time"] = link_timeline_by | ||
|
|
||
| column_mapping["links"] = { | ||
| "columns": links_columns, | ||
| "tableName": "link_table", | ||
| } | ||
|
|
||
| try: | ||
| config_json = { | ||
| "json": { | ||
| "apiKey": api_key, | ||
| "config": { | ||
| "title": project_name, | ||
| "dataSources": data_sources, | ||
| "columnMapping": column_mapping, | ||
| "cosmographConfig": cosmograph_config, | ||
| }, | ||
| }, | ||
| } | ||
| # Create a version without API key for logging | ||
| if debug: | ||
| config_for_logging = { | ||
| "json": { | ||
| "apiKey": "***", | ||
| "config": config_json["json"]["config"] | ||
| } | ||
| } | ||
| logger.info("📋 Full project config being sent: %s", json.dumps(config_for_logging, indent=2)) | ||
| start_time = time.perf_counter() if debug else None | ||
| response = requests.post( | ||
| f"{API_BASE}/publicApi.upsertProjectByName", | ||
| json=config_json, | ||
| ) | ||
| response.raise_for_status() | ||
| if debug: | ||
| logger.info("⏱️ - upsertProjectByName API request took: %.3f seconds", time.perf_counter() - start_time) | ||
|
|
||
| return response.json() | ||
| except requests.RequestException as e: | ||
| logger.error("❌ Failed to create project '%s': %s", project_name, e) | ||
| msg = f"Failed to create project: {e}" | ||
| raise ValueError(msg) from e |
There was a problem hiding this comment.
Add timeout and input validation.
The create_project function needs timeout and validation improvements:
- Missing timeout: Request could hang indefinitely
- No input validation: Should validate
api_key,project_name, and require at least one data source
Apply these changes:
def create_project(
api_key: str,
project_name: str,
points_data: Optional[dict[str, Any]] = None,
links_data: Optional[dict[str, Any]] = None,
cosmograph_config: Optional[dict[str, Any]] = None,
debug: bool = False,
) -> dict[str, Any]:
"""Create a new Cosmograph project with the uploaded files.
Raises:
ValueError: If project creation fails
Args:
api_key: Cosmograph API key
project_name: Name for the project
points_data: Data for points table
links_data: Data for links table
cosmograph_config: Cosmograph config
Returns:
dict: API response from project creation
"""
+ if not api_key or not api_key.strip():
+ raise ValueError("API key cannot be empty")
+ if not project_name or not project_name.strip():
+ raise ValueError("Project name cannot be empty")
+ if not points_data and not links_data:
+ raise ValueError("At least one of points_data or links_data must be provided")
cosmograph_config = cosmograph_config or {}And add timeout to the request:
response = requests.post(
f"{API_BASE}/publicApi.upsertProjectByName",
json=config_json,
+ timeout=30,
)Based on learnings
🤖 Prompt for AI Agents
In cosmograph/widget/export_project/create_project.py around lines 59 to 213,
add input validation and a request timeout: validate that api_key and
project_name are non-empty strings and raise ValueError with clear messages if
not; ensure at least one of points_data or links_data is provided (raise
ValueError if both are None); then add a timeout argument to the requests.post
call (e.g., timeout=30) and catch requests.Timeout separately if desired to
log/raise a specific ValueError, ensuring the request cannot hang indefinitely.
| try: | ||
| start_time = time.perf_counter() if debug else None | ||
| response = requests.post( | ||
| f"{API_BASE}/publicApi.generateSignedUploadUrl", | ||
| json={ | ||
| "json": { | ||
| "apiKey": api_key, | ||
| "projectId": project_id, | ||
| "fileName": data["file_name"], | ||
| "contentLength": data["content_length"], | ||
| "contentType": "application/parquet" | ||
| }, | ||
| }, | ||
| ) | ||
| response.raise_for_status() | ||
| if debug: | ||
| logger.info("⏱️ - Generate signed URL API request took: %.3f seconds", time.perf_counter() - start_time) | ||
| # logger.info("Response: %s", json.dumps(response.json(), indent=4)) | ||
| response_json = response.json() | ||
| try: | ||
| upload_url = response_json["result"]["data"]["json"]["url"] | ||
| except (KeyError, TypeError) as e: | ||
| logger.error("❌ Unexpected response format: %s", response_json) | ||
| msg = f"Failed to parse upload URL from response: {e}" | ||
| raise ValueError(msg) from e | ||
| except requests.RequestException as e: | ||
| logger.error("❌ Failed to get upload URL: %s", e) | ||
| msg = f"Failed to get upload URL: {e}" | ||
| raise ValueError(msg) from e |
There was a problem hiding this comment.
Add timeout to prevent hanging requests.
The signed URL generation request lacks a timeout parameter, which could cause the operation to hang indefinitely if the server is unresponsive.
Apply this diff:
response = requests.post(
f"{API_BASE}/publicApi.generateSignedUploadUrl",
json={
"json": {
"apiKey": api_key,
"projectId": project_id,
"fileName": data["file_name"],
"contentLength": data["content_length"],
"contentType": "application/parquet"
},
},
+ timeout=30,
)Based on learnings
🤖 Prompt for AI Agents
cosmograph/widget/export_project/upload_file.py around lines 27-55: the
requests.post call for generating the signed upload URL has no timeout which can
cause the process to hang; update the requests.post invocation to include a
timeout (e.g., timeout=10 or use a configurable timeout variable) so the call
fails fast on network issues, and ensure the surrounding exception handling
(requests.RequestException) remains valid to catch the Timeout exception and
convert it to the existing ValueError flow.
| try: | ||
| start_time = time.perf_counter() if debug else None | ||
| upload_response = requests.put( | ||
| upload_url, | ||
| data=data["content"], | ||
| headers={"Content-Type": "application/parquet"} | ||
| ) | ||
| upload_response.raise_for_status() | ||
| logger.info("✅ File '%s' upload completed successfully", data["file_name"]) | ||
| if debug: | ||
| upload_time = time.perf_counter() - start_time | ||
| file_size_mb = data["content_length"] / (1024 * 1024) | ||
| logger.info("⏱️ - File upload (%.2f MB) took: %.3f seconds", file_size_mb, upload_time) | ||
| except requests.RequestException as e: | ||
| logger.error("❌ Failed to upload file '%s': %s", data["file_name"], e) | ||
| msg = f"Failed to upload file: {e}" | ||
| raise ValueError(msg) from e |
There was a problem hiding this comment.
Add timeout for file upload request.
File uploads lack a timeout, risking indefinite hangs. Large files need longer timeouts than API calls, but still require bounds.
Apply this diff:
upload_response = requests.put(
upload_url,
data=data["content"],
- headers={"Content-Type": "application/parquet"}
+ headers={"Content-Type": "application/parquet"},
+ timeout=300, # 5 minutes for large file uploads
)Based on learnings
🤖 Prompt for AI Agents
In cosmograph/widget/export_project/upload_file.py around lines 57 to 73, the
requests.put call has no timeout which can cause indefinite hangs for large
uploads; add a timeout parameter (use a tuple for connect and read, e.g.,
timeout=(10, 600) or a configurable value) to the requests.put call so the
upload has bounded wait time, propagate the timeout into error handling (catch
requests.Timeout if needed) and ensure logging/raised ValueError includes
timeout errors the same as other RequestExceptions.
cosmograph/__about__.py
Outdated
| """Package metadata for cosmograph""" | ||
|
|
||
| __version__ = "0.0.47" | ||
| __version__ = "0.5.0" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify version choice: "0.5.0" orders lower than previously discussed "1.0.0b0".
The previous review discussed version "1.0.0b0", but the current version is "0.5.0". Under PEP 440, "0.5.0" sorts before "1.0.0b0", which could cause issues if "1.0.0b0" was already published or tagged. Additionally, this PR introduces substantial new features (API key management, export_project module, expanded API surface), which typically warrant a more significant version bump.
Please confirm:
- Is "0.5.0" intentional, or should this be "1.0.0" (or higher) given the feature scope?
- Has "1.0.0b0" been published/tagged? If so, users who installed it would see "0.5.0" as a downgrade.
- Does integrating with Cosmograph v2.0.0-beta.25 (upstream beta) while releasing a stable "0.5.0" align with your stability guarantees?
Run this script to check for existing version tags and validate the ordering:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==> Check for existing version tags in repository"
git tag -l | grep -E '^(v)?[0-9]+\.[0-9]+\.[0-9]' | sort -V | tail -10
echo
echo "==> Validate PEP 440 ordering"
python - <<'PY'
from packaging.version import Version
versions = ["0.0.47", "0.5.0", "1.0.0b0", "1.0.0"]
parsed = [(v, Version(v)) for v in versions]
sorted_versions = sorted(parsed, key=lambda x: x[1])
print("Version ordering:")
for v, parsed_v in sorted_versions:
print(f" {v:12} -> {parsed_v.public}")
current = Version("0.5.0")
previous_discussed = Version("1.0.0b0")
if current < previous_discussed:
print(f"\n⚠️ Current version {current} sorts BEFORE {previous_discussed}")
PY
echo
echo "==> Check if version has been published to PyPI"
curl -s https://pypi.org/pypi/cosmograph/json | jq -r '.releases | keys[]' | grep -E '^(0\.5\.0|1\.0\.0b0)' || echo "Neither version found on PyPI"Length of output: 118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==> Check for existing version tags in repository"
git tag -l | grep -E '^(v)?[0-9]+\.[0-9]+\.[0-9]' | sort -V | tail -15 || echo "No matching version tags found"
echo
echo "==> Check git log for version changes"
git log --oneline --all -- cosmograph/__about__.py | head -20 || echo "Could not retrieve git history"
echo
echo "==> Check if version has been published to PyPI"
curl -s https://pypi.org/pypi/cosmograph/json 2>/dev/null | jq -r '.releases | keys[] | select(. == "0.5.0" or . == "1.0.0b0")' || echo "Could not query PyPI"
echo
echo "==> List recent PyPI releases"
curl -s https://pypi.org/pypi/cosmograph/json 2>/dev/null | jq -r '.releases | keys[] | select(. | test("^(0\\.|1\\.)"))' | sort -rV | head -10 || echo "Could not query PyPI"Length of output: 497
Version regression detected: 0.5.0 must be bumped to 1.0.0 or higher.
The git history and PyPI data confirm a critical issue: version "1.0.0b1" was previously released, and the current "0.5.0" is lower in version ordering. This creates a version regression where users and package managers will treat 0.5.0 as an older release than the already-published 1.0.0b0 and 1.0.0b1.
PyPI release history shows:
- 1.0.0b1 (published)
- 1.0.0b0 (published)
- 0.5.0b0
- 0.5.0 (current)
This will break dependency resolution and confuse users. The version must be changed to at least "1.0.0" (or higher if warranted by the feature scope in this PR). Do not release 0.5.0 as-is.
🤖 Prompt for AI Agents
In cosmograph/__about__.py around line 3, the package version is set to "0.5.0"
which regresses relative to previously published "1.0.0b1"; update the
__version__ value to "1.0.0" or a higher appropriate stable version (e.g.,
"1.0.1" if additional changes warrant) so the new value is strictly greater than
prior 1.0.0b1, and ensure any associated packaging metadata
(pyproject.toml/setup.cfg) and changelog entries are updated to match.
- Rename simulation_cluster_strength → simulation_cluster - Add reset_selection_on_empty_canvas_click parameter - Add selected_link_indices property - Remove deprecated show_focused_point_label parameter - Update point_color_palette to include linkDirection strategy
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cosmograph/data/params_ssot.json (1)
297-301: Fix SSOT parameter name: show_fps_monitor → show_FPS_monitorThe widget trait is named
show_FPS_monitor(uppercase FPS) but the SSOT defines it asshow_fps_monitor, causing downstream signature generation to use the wrong name.Apply this diff:
{ - "name":"show_fps_monitor", + "name":"show_FPS_monitor", "default":false, "annotation":"bool", "description":"Display an FPS counter in the upper right corner of the canvas." },
♻️ Duplicate comments (5)
js/config-props.json (1)
97-114: Add missing mapping: show_focused_point_label.Without this, Python’s show_focused_point_label won’t propagate to JS config.
"show_top_labels_limit": "showTopLabelsLimit", "show_cluster_labels": "showClusterLabels", + "show_focused_point_label": "showFocusedPointLabel", "point_label_color": "pointLabelColor",js/widget.ts (1)
66-68: Fix runtime typo: unselectPointsByIndicies → unselectPointsByIndices.Current call will throw (method doesn’t exist).
- if (msg.type === 'unselect_points_by_indices') { - cosmograph?.unselectPointsByIndicies(msg.indices) - } + if (msg.type === 'unselect_points_by_indices') { + cosmograph?.unselectPointsByIndices(msg.indices) + }cosmograph/data/params_ssot.json (1)
405-409: Type definition is correct; verify generated docs are in sync.The SSOT correctly defines
point_color_by_mapasDict[str, Union[str, list[float]]]. If generated documentation or signatures still show the old list type, re-run the SSOT→docs pipeline as noted in past reviews.cosmograph/base.py (1)
113-113: Fix param name mismatch: show_fps_monitor → show_FPS_monitorThe widget trait is
show_FPS_monitor(uppercase FPS) butcosmo()acceptsshow_fps_monitor(lowercase), causing an unknown trait error when constructing the Cosmograph widget.Apply this diff:
- show_fps_monitor: bool = None, + show_FPS_monitor: bool = None,cosmograph/widget/__init__.py (1)
218-218: Fix mutable default for cosmograph_config to prevent cross-instance state leakage.Using
default_value={}can cause the same dict to be shared across instances. UseNoneand initialize per-instance.Apply this diff:
- cosmograph_config = Dict(default_value={}, allow_none=True).tag(sync=True) + cosmograph_config = Dict(default_value=None, allow_none=True).tag(sync=True)Then in
__init__after line 346, add:if self.api_key is None: global_api_key = get_api_key() if global_api_key is not None: self.api_key = global_api_key + + # Ensure per-instance dict (avoid shared mutable default) + if self.cosmograph_config is None: + self.cosmograph_config = {}
🧹 Nitpick comments (3)
package.json (1)
5-6: Confirm library API compatibility; optionally enforce Node version.
- Verify that Cosmograph ^2.0.0-beta.28 includes the callbacks used in js/widget.ts (onGraphDataUpdated, onGraphRebuilt).
- Consider adding an engines field to avoid CI/environment drift with ESLint 9/TS 5/esbuild 0.23.
Example package fragment:
{ "type": "module", "scripts": { "clean": "rm -rf cosmograph/widget/static", "build:esbuild": "esbuild js/widget.ts --minify --bundle --splitting --entry-names=[name]-[hash] --metafile=meta.json --format=esm --outdir=cosmograph/widget/static", "build:meta": "node create-meta-file.cjs", @@ "devDependencies": { @@ } + ,"engines": { + "node": ">=18.18" + } }Also applies to: 13-13
js/widget.ts (1)
36-44: Unsubscribe msg:custom handler to avoid leaks on re-render.Store handler and remove in cleanup.
- model.on('msg:custom', async (msg: { [key: string]: never }) => { + const onCustomMsg = async (msg: { [key: string]: never }) => { if (msg.type === 'select_point_by_index') { cosmograph?.selectPoint(msg.index, true) } @@ - }) + } + model.on('msg:custom', onCustomMsg) @@ return (): void => { unsubscribes.forEach(unsubscribe => unsubscribe()) cosmograph?.destroy() settingsPanel?.destroy() pointInfoPanel?.destroy() + model.off('msg:custom', onCustomMsg) }Also applies to: 296-301
demos/colab_Mobius_in_Cosmograph_Widget.ipynb (1)
44-52: Handle API keys safely in notebooks.Avoid hardcoding secrets; use env vars or input prompt.
import os from getpass import getpass api_key = os.getenv("COSMOGRAPH_API_KEY") or getpass("Enter Cosmograph API key: ") set_api_key(api_key)Also applies to: 282-291
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
cosmograph/base.py(10 hunks)cosmograph/data/params_ssot.json(11 hunks)cosmograph/widget/__init__.py(8 hunks)demos/colab_Mobius_in_Cosmograph_Widget.ipynb(7 hunks)js/config-props.json(1 hunks)js/legends.ts(2 hunks)js/widget.ts(4 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
cosmograph/base.py (3)
cosmograph/util.py (1)
add_cosmo_param_descriptions(111-135)cosmograph/config.py (1)
get_api_key(23-30)cosmograph/widget/__init__.py (1)
Cosmograph(26-357)
js/legends.ts (1)
js/helper.ts (2)
isDuckDBNumericType(16-32)isDuckDBStringType(34-38)
js/widget.ts (9)
js/widget-elements.ts (5)
createWidgetContainer(1-20)createLoadingOverlay(22-33)showLoadingOverlay(35-38)setLoadingOverlayBackgroundColor(47-60)hideLoadingOverlay(40-45)cosmograph/widget/__init__.py (1)
Cosmograph(26-357)js/components/point-timeline.ts (1)
PointTimeline(4-43)js/components/link-timeline.ts (1)
LinkTimeline(4-44)js/components/settings-panel.ts (1)
SettingsPanel(10-355)js/components/point-info.ts (1)
PointInfoPanel(3-144)js/helper.ts (1)
subscribe(11-14)js/cosmograph-data.ts (1)
prepareCosmographDataAndMutate(12-90)js/components/control-buttons.ts (1)
ControlButtonsComponent(4-23)
cosmograph/widget/__init__.py (4)
cosmograph/_dev_utils/_resources.py (2)
traitlets(409-416)traitlets(1243-1249)cosmograph/widget/utils.py (1)
get_buffered_arrow_table(17-36)cosmograph/widget/export_project/__init__.py (1)
export_project(70-191)cosmograph/config.py (3)
register_instance(33-40)unregister_instance(43-51)get_api_key(23-30)
⏰ 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). (2)
- GitHub Check: Windows Validation (3.10)
- GitHub Check: Windows Validation (3.10)
🔇 Additional comments (10)
js/widget.ts (1)
13-14: Type-check JSON import.Ensure tsconfig enables resolveJsonModule (and compatible moduleResolution) so tsc --noEmit passes for JSON import.
Add to tsconfig.json:
{ "compilerOptions": { "resolveJsonModule": true, "moduleResolution": "bundler" } }js/legends.ts (1)
19-32: Legends handling looks solid.Good use of optional chaining, scoped switch cases, and safe show/hide wrappers.
Also applies to: 101-125, 153-167
cosmograph/base.py (2)
184-187: LGTM: API key injection logic is correct.The global API key fallback is properly implemented, respecting explicit None values while providing sensible defaults.
443-449: LGTM: Deprecation warning properly implemented.The deprecation notice is correctly placed in both the docstring and runtime warning. The
stacklevel=2should correctly point to the caller's code.cosmograph/data/params_ssot.json (1)
8-67: LGTM: New parameter definitions are well-documented.All new parameters include proper types, defaults, and descriptions. The expanded surface for polygonal selection, clustering, labeling, and timeline features is clearly defined.
Also applies to: 314-319, 428-433, 471-475, 548-553, 585-655, 717-721
cosmograph/widget/__init__.py (5)
333-346: LGTM: Initialization properly handles registration and API key fallback.The lifecycle hook correctly registers the instance for API key updates with proper error handling, and applies the global API key fallback when needed.
348-357: LGTM: Cleanup properly handles unregistration and super() call.The destructor safely unregisters the instance and only calls
super().__del__()if it exists, addressing past review concerns.
323-331: LGTM: Export method properly validates inputs and handles errors.The validation and error handling address all past review concerns. Parameters are checked before calling the export pipeline, and exceptions are wrapped with helpful context.
258-264: LGTM: Polygon validation is thorough and clear.The validation properly checks polygon structure and raises helpful error messages, addressing past review concerns.
90-92: LGTM: New widget traits properly defined.All new traits use appropriate types, sync tags, and allow_none flags. The expanded configuration surface is correctly implemented.
Also applies to: 108-110, 120-122, 127-127, 134-136, 156-156, 166-195, 220-220
| // Show point info panel | ||
| pointInfoPanel?.showPointInfo(index ?? null) | ||
| }, |
There was a problem hiding this comment.
Guard PointInfo panel for null clicks.
Passing null to showPointInfo likely errors (expects a number). Hide panel when no point selected.
- // Show point info panel
- pointInfoPanel?.showPointInfo(index ?? null)
+ // Show/hide point info panel
+ if (index != null) pointInfoPanel?.showPointInfo(index)
+ else pointInfoPanel?.hide()🤖 Prompt for AI Agents
In js/widget.ts around lines 119 to 121, the call
pointInfoPanel?.showPointInfo(index ?? null) passes null when no point is
selected which likely throws because showPointInfo expects a number; update the
code to check whether index is a valid number before calling showPointInfo (e.g.
if typeof index === 'number' and not NaN then call
pointInfoPanel.showPointInfo(index)), otherwise call
pointInfoPanel.hidePointInfo() (or pointInfoPanel.hide()) to hide the panel when
no point is selected.
| const unsubscribes = Object | ||
| .entries(modelChangeHandlers) | ||
| .map(([propName, onModelChange]) => subscribe(model, `change:${propName}`, async () => { | ||
| .map(([snakeCaseProp, onModelChange]) => subscribe(model, `change:${snakeCaseProp}`, async () => { | ||
| onModelChange() | ||
|
|
||
| // If color associated properties change, update the color strategy | ||
| if (propName === 'point_color_by' || propName === 'point_color_palette' || propName === 'point_color_by_map' || propName === 'point_color_strategy') { | ||
| const pointsSummary = cosmograph?.stats.pointsSummary | ||
| cosmographConfig.pointColorStrategy = model.get('point_color_strategy') ?? resolveOptimalPointColorStrategy(cosmographConfig, pointsSummary) | ||
| } | ||
|
|
||
| // If size associated properties change, update the size strategy | ||
| if (propName === 'point_size_by' || propName === 'point_size_range' || propName === 'point_size_strategy') { | ||
| const pointsSummary = cosmograph?.stats.pointsSummary | ||
| cosmographConfig.pointSizeStrategy = model.get('point_size_strategy') ?? resolveOptimalPointSizeStrategy(cosmographConfig, pointsSummary) | ||
| } | ||
|
|
||
| if (propName === 'link_color_by') { | ||
| if (cosmograph?.stats.linksSummary) updateLinkColorFn(cosmograph.stats.linksSummary, cosmographConfig) | ||
| } | ||
|
|
||
| if (configProperties.includes(propName)) { | ||
| await cosmograph?.setConfig(cosmographConfig) | ||
| if (snakeToCamelConfigProps[snakeCaseProp as keyof typeof snakeToCamelConfigProps]) { | ||
| cosmograph?.setConfig(cosmographConfig) | ||
| // await when config is set | ||
| await cosmograph?.getConfig() | ||
| updatePythonCosmographConfig() | ||
| } |
There was a problem hiding this comment.
Data updates don’t reach Cosmograph (points/links).
Subscribers only call setConfig for mapped props; _ipc_points/_ipc_links aren’t mapped, so live data changes won’t render.
- .map(([snakeCaseProp, onModelChange]) => subscribe(model, `change:${snakeCaseProp}`, async () => {
+ .map(([snakeCaseProp, onModelChange]) => subscribe(model, `change:${snakeCaseProp}`, async () => {
onModelChange()
- if (snakeToCamelConfigProps[snakeCaseProp as keyof typeof snakeToCamelConfigProps]) {
+ const isConfigProp =
+ !!snakeToCamelConfigProps[snakeCaseProp as keyof typeof snakeToCamelConfigProps]
+ const isDataProp = snakeCaseProp === '_ipc_points' || snakeCaseProp === '_ipc_links'
+
+ if (isConfigProp || isDataProp) {
cosmograph?.setConfig(cosmographConfig)
// await when config is set
await cosmograph?.getConfig()
updatePythonCosmographConfig()
}Also applies to: 147-154
🤖 Prompt for AI Agents
In js/widget.ts around lines 183-193 (and also apply the same change for
147-154), subscribers currently only call
cosmograph.setConfig/getConfig/updatePythonCosmographConfig when the changed
prop exists in snakeToCamelConfigProps, which skips unmapped keys like
_ipc_points/_ipc_links so point/link updates never reach Cosmograph; update the
subscriber logic to also call cosmograph.setConfig, await
cosmograph.getConfig(), and then updatePythonCosmographConfig() when the changed
prop is an IPC data prop (e.g., keys starting with "_ipc_") or when any
config-affecting prop changes (alternatively, include _ipc_points and _ipc_links
in snakeToCamelConfigProps), ensuring the set/get calls are awaited and invoked
for those data updates so live points/links render.
2 participants
Upgraded from Cosmograph v1.x to v2.0.0-beta.25
Added global API key configuration and new export_project module
set_api_key()- Set global API key for all instancesget_api_key()- Retrieve current API keyexport_project()- Export visualizations to Cosmograph platformExample Usage
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation
Tests