Skip to content

0.5.0 exp#4

Open
Arenukvern wants to merge 3 commits intomasterfrom
0.5.0-exp
Open

0.5.0 exp#4
Arenukvern wants to merge 3 commits intomasterfrom
0.5.0-exp

Conversation

@Arenukvern
Copy link
Copy Markdown
Member

@Arenukvern Arenukvern commented May 31, 2025

closes #3 and updates to Dart 3.7

Summary by CodeRabbit

  • New Features

    • Added detailed configuration and guidelines files for Dart, TypeScript, and Flutter UI development, including coding standards, documentation practices, and strategic coding rules.
  • Bug Fixes

    • Improved handling and conversion between Dart and JavaScript types for Office.js interop, ensuring more reliable data exchange and type safety.
  • Refactor

    • Migrated all Office.js interop code to use the modern dart:js_interop API, replacing deprecated patterns and updating type signatures for better maintainability and compatibility.
    • Simplified factory constructors and method implementations for improved readability and consistency.
  • Chores

    • Updated Dart SDK constraint to require version 3.7.0 or higher.
    • Removed obsolete dependencies and configuration files.
    • Adjusted editor settings for Dart word-based suggestions.
  • Documentation

    • Added and updated changelogs and migration plans to reflect recent upgrades and codebase changes.

@Arenukvern Arenukvern self-assigned this May 31, 2025
@Arenukvern Arenukvern marked this pull request as ready for review May 31, 2025 02:34
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 31, 2025

Walkthrough

The codebase was refactored to migrate from the deprecated package:js and related dynamic interop utilities to Dart's modern dart:js_interop static interop API. This involved updating interop classes, method and property signatures, and utility functions to use explicit JS types and conversions. Additional configuration and documentation files were also added or updated.

Changes

File(s) Change Summary
.cursor/rules/dart_dev.mdc, .cursor/rules/flutter_ui_dev.mdc, .cursor/rules/think_hard.mdc, .cursor/rules/typescript.mdc Added new markdown configuration files defining coding standards and rules for Dart, Flutter, and TypeScript development.
CHANGELOG.md, PLAN.md Added or updated documentation describing the migration plan and changelog for the JS interop refactor.
.fvm/fvm_config.json Deleted Flutter version manager configuration file.
.vscode/settings.json Changed Dart editor word suggestion setting from false to "matchingDocuments".
pubspec.yaml Updated Dart SDK constraint to >=3.7.0 <4.0.0, removed js package dependency, and adjusted Flutter config.
lib/officejs.dart Removed redundant library directive and blank line.
lib/src/js_interops/es6_js_impl.dart, lib/src/js_interops/office_helpers_js_impl.dart Migrated from package:js to dart:js_interop, refactored interop classes and method signatures for static interop.
lib/src/office/excel.dart, lib/src/office/office.dart, lib/src/office/office_extension.dart Updated to use new static interop classes and explicit type conversions between Dart and JS objects.
lib/src/office_interops/excel_js_impl.dart, lib/src/office_interops/office_core_js_impl.dart, lib/src/office_interops/office_extension_js_impl.dart, lib/src/office_interops/office_js_impl.dart Refactored all Office/Excel JS interop bindings to use @staticInterop, extensions, and JS types.
lib/src/utils/interop_utils.dart Rewrote interop utility functions to use dart:js_interop and explicit conversions, added dartify function.

Sequence Diagram(s)

sequenceDiagram
    participant DartCode
    participant JSInteropClass
    participant JSObject

    DartCode->>JSInteropClass: Call staticInterop method (with .toJS/.toDart conversions)
    JSInteropClass->>JSObject: Access or invoke JS property/method
    JSObject-->>JSInteropClass: Return JS value (JSString, JSNumber, etc.)
    JSInteropClass-->>DartCode: Return value (converted to Dart type)
Loading

Assessment against linked issues

Objective (Issue #) Addressed Explanation
Migrate to new dart:js_interop API (#3)
Refactor interop classes, methods, and signatures (#3)
Remove deprecated package:js usage (#3)
Update utility functions for new interop (#3)

Poem

In the garden of code, a bunny leaps high,
From old JS vines to new interop sky.
With static and types, the carrots align,
Dart and JS now perfectly entwine.
🥕✨
Here's to fresh code and bugs left behind—
The rabbit hops on, with a peace of mind!


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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🔭 Outside diff range comments (1)
lib/src/office/office_extension.dart (1)

89-123: ⚠️ Potential issue

The remove method won't work correctly due to function reference mismatch.

The current implementation creates a new callback function in both add and remove methods. Since JavaScript/Dart compare functions by reference, the remove method will create a different function instance that won't match the one added, making it impossible to remove event handlers.

Consider storing the JS callback references to enable proper removal:

class EventHandlers<T>
    extends JsObjectWrapper<office_extension_js.EventHandlersJsImpl> {
  EventHandlers._fromJsObject(
    super.jsObject, {
    required this.fromJson,
  });
+  
+  final _callbackMap = <void Function(T), JSFunction>{};

  EventHandlerResult<T> add(
    final void Function(T args) handler,
  ) {
    void promiseCallback(final JSAny? json) {
      final dartJson = dartify(json)! as Map<String, dynamic>;
      handler(fromJson(dartJson));
    }

-    final resultJs = jsObject.add(promiseCallback.toJS);
+    final jsCallback = promiseCallback.toJS;
+    _callbackMap[handler] = jsCallback;
+    final resultJs = jsObject.add(jsCallback);
    return EventHandlerResult.getInstance(resultJs);
  }

  void remove(final void Function(T args) handler) {
-    void promiseCallback(final JSAny? json) {
-      final dartJson = dartify(json)! as Map<String, dynamic>;
-      handler(fromJson(dartJson));
-    }
-
-    jsObject.remove(promiseCallback.toJS);
+    final jsCallback = _callbackMap.remove(handler);
+    if (jsCallback != null) {
+      jsObject.remove(jsCallback);
+    }
  }
}
🧹 Nitpick comments (11)
.cursor/rules/think_hard.mdc (1)

6-9: Fix grammatical issues in the strategic guidelines.

The strategic approach outlined here is excellent, but there are some grammatical issues that should be corrected for clarity and professionalism.

-You are great strateg.
+You are a great strategist.
 - Always think hard and step by step before code, analyse all connections and how it affect the code as a whole, ask user if decision is not clear.
-- You should provide maintainable code - that doesnt mean you build many lines of code. Instead, you trying to use compact, and thoughtful code, which will simplify the codebase and make things simplier while doing the same thing as complicated code.
+- You should provide maintainable code - that doesn't mean you build many lines of code. Instead, you try to use compact and thoughtful code, which will simplify the codebase and make things simpler while doing the same thing as complicated code.
 - Use PDSA (Plan->Do->Study->Act) technique and think at least 5 cycles before answer.
.cursor/rules/typescript.mdc (1)

86-90: Minor formatting issue at the end of the file.

The TypeScript guidelines are comprehensive and well-structured. However, there's a formatting inconsistency at the end.

 ## Git Commit Rules
 - Make the head / title of the commit message brief
 - Include elaborate details in the body of the commit message
 - Always follow the conventional commit message format
 - Add two newlines after the commit message title
-90
+
.cursor/rules/dart_dev.mdc (2)

9-9: Fix duplicate section numbering.

The document has duplicate "2." numbering for different sections. The second occurrence should be numbered differently to maintain proper document structure.

-2. Coding standards:
+3. Coding standards:

And update subsequent sections accordingly:

-3. Flutter widgets and concepts:
+4. Flutter widgets and concepts:
-4. Project structure:
+5. Project structure:
-5. Additional requirements:
+6. Additional requirements:

Also applies to: 39-39


72-72: Add missing newline at end of file.

The file should end with a newline character for proper Unix text file formatting.

 Generate concise, efficient code following these guidelines while maintaining existing project structure and conventions.
+
lib/src/utils/interop_utils.dart (1)

29-31: Remove unnecessary cast to JSAny?

The cast on line 30 is redundant since TimestampJsImpl should already be a valid JSAny type through the JS interop type hierarchy.

-    return js.TimestampJsImpl(dartObject.millisecondsSinceEpoch.toJS, 0.toJS)
-        as JSAny?;
+    return js.TimestampJsImpl(dartObject.millisecondsSinceEpoch.toJS, 0.toJS);
PLAN.md (5)

1-2: Use proper markdown heading instead of emphasis.

Replace the emphasized text with a proper markdown heading for better document structure.

-**Revised Step-by-Step Plan**
+# Revised Step-by-Step Plan
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

1-1: Emphasis used instead of a heading
null

(MD036, no-emphasis-as-heading)


3-3: Format the bare URL as a proper markdown link.

Bare URLs should be formatted as markdown links for better readability.

-Before start: read the https://dart.dev/interop/js-interop to understand the js interop in dart.
+Before start: read the [Dart JavaScript Interoperability guide](https://dart.dev/interop/js-interop) to understand the js interop in dart.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

3-3: Bare URL used
null

(MD034, no-bare-urls)


5-5: Use proper markdown heading for phase title.

-**Phase 1: Dart SDK Upgrade to 3.9.0**
+## Phase 1: Dart SDK Upgrade to 3.9.0
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

5-5: Emphasis used instead of a heading
null

(MD036, no-emphasis-as-heading)


15-16: Use proper markdown heading for phase title.

-**Phase 2: Office JS API SDK Migration to `dart:js_interop`**
+## Phase 2: Office JS API SDK Migration to `dart:js_interop`

49-49: Remove extra spaces inside code span.

-                - `query`: e.g., `class Worksheet `, `interface Range {`, `enum BindingType {` (note the space or opening brace to target definitions)
+                - `query`: e.g., `class Worksheet`, `interface Range {`, `enum BindingType {` (note the space or opening brace to target definitions)
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

49-49: Spaces inside code span elements
null

(MD038, no-space-in-code)

lib/src/office/excel.dart (1)

100-108: Consider simplifying array conversion

The explicit for-loop works correctly but could be more concise using functional programming patterns.

Consider using map for a more idiomatic Dart approach:

  List<Worksheet> get items {
    final jsItems = jsObject.items;
-   final dartItems = <Worksheet>[];
-   for (int i = 0; i < jsItems.length; i++) {
-     final item = jsItems[i] as excel_js.WorksheetJsImpl;
-     dartItems.add(Worksheet.getInstance(item));
-   }
-   return dartItems;
+   return List.generate(
+     jsItems.length,
+     (i) => Worksheet.getInstance(jsItems[i] as excel_js.WorksheetJsImpl),
+   );
  }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b9a4d4 and 0061d70.

📒 Files selected for processing (20)
  • .cursor/rules/dart_dev.mdc (1 hunks)
  • .cursor/rules/flutter_ui_dev.mdc (1 hunks)
  • .cursor/rules/think_hard.mdc (1 hunks)
  • .cursor/rules/typescript.mdc (1 hunks)
  • .fvm/fvm_config.json (0 hunks)
  • .vscode/settings.json (1 hunks)
  • CHANGELOG.md (1 hunks)
  • PLAN.md (1 hunks)
  • lib/officejs.dart (0 hunks)
  • lib/src/js_interops/es6_js_impl.dart (1 hunks)
  • lib/src/js_interops/office_helpers_js_impl.dart (4 hunks)
  • lib/src/office/excel.dart (12 hunks)
  • lib/src/office/office.dart (5 hunks)
  • lib/src/office/office_extension.dart (8 hunks)
  • lib/src/office_interops/excel_js_impl.dart (11 hunks)
  • lib/src/office_interops/office_core_js_impl.dart (1 hunks)
  • lib/src/office_interops/office_extension_js_impl.dart (3 hunks)
  • lib/src/office_interops/office_js_impl.dart (4 hunks)
  • lib/src/utils/interop_utils.dart (1 hunks)
  • pubspec.yaml (2 hunks)
💤 Files with no reviewable changes (2)
  • lib/officejs.dart
  • .fvm/fvm_config.json
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
PLAN.md

1-1: Emphasis used instead of a heading
null

(MD036, no-emphasis-as-heading)


3-3: Bare URL used
null

(MD034, no-bare-urls)


5-5: Emphasis used instead of a heading
null

(MD036, no-emphasis-as-heading)


49-49: Spaces inside code span elements
null

(MD038, no-space-in-code)

🔇 Additional comments (25)
.vscode/settings.json (1)

12-12: LGTM: Improved editor experience for Dart development.

This change enhances code completion by enabling word-based suggestions from matching documents, which is a sensible configuration for Dart development.

pubspec.yaml (2)

28-28: LGTM: Flutter configuration normalized.

The change from flutter: null to flutter: {} is a cosmetic improvement that maintains the same semantic meaning while using a more standard YAML representation.


9-9:

Details

❓ Verification inconclusive

Verify dependency compatibility with Dart 3.7+.

This SDK constraint update to require Dart 3.7+ is a breaking change that aligns with the migration to dart:js_interop. However, ensure all dependencies are compatible with this version requirement.


🏁 Script executed:

#!/bin/bash
# Description: Check if all dependencies support Dart 3.7+
# Expected: All dependencies should have compatible version ranges

# Check dependency compatibility
echo "Checking dependency compatibility with Dart 3.7+:"
echo

# Extract dependencies and check their pub.dev constraints
dependencies=(
  "flutter"
  "freezed_annotation:^2.0.3"
  "build_runner:^2.1.11" 
  "flutter_lints:^2.0.1"
  "flutter_test"
  "freezed:^2.0.3+1"
  "json_serializable:^6.2.0"
  "xsoulspace_lints:^0.0.5"
)

for dep in "${dependencies[@]}"; do
  if [[ "$dep" == "flutter" ]] || [[ "$dep" == "flutter_test" ]]; then
    echo "$dep: SDK dependency, follows Flutter's Dart requirements"
  else
    package_name=$(echo "$dep" | cut -d: -f1)
    echo "Checking $package_name on pub.dev..."
    curl -s "https://pub.dev/api/packages/$package_name" | jq -r '.latest.pubspec.environment.sdk // "No SDK constraint found"'
  fi
done

Length of output: 3402


Dependency check: flutter_lints requires Dart ≥3.8.0

We’ve verified SDK constraints on pub.dev for your pinned dependencies:

  • freezed_annotation (latest): >=3.0.0 <4.0.0 → supports 3.7+
  • build_runner (latest): ^3.6.0 → supports 3.7+
  • freezed (latest): >=3.6.0 <4.0.0 → supports 3.7+
  • json_serializable (latest): ^3.6.0 → supports 3.7+
  • xsoulspace_lints (latest): >=3.7.0 <4.0.0 → supports 3.7+
  • flutter_lints (latest): ^3.8.0 → requires Dart ≥3.8.0

Given your SDK constraint of ">=3.7.0 <4.0.0", flutter_lints may not resolve on 3.7.x. Please verify and adjust accordingly:

  • Check that version 2.0.1 of flutter_lints (your pinned version) has the same SDK requirement.
  • If you must support Dart 3.7.x, either downgrade flutter_lints to a 3.7-compatible release or raise your SDK lower bound to >=3.8.0.

Location:

  • pubspec.yaml: line 9
lib/src/office_interops/office_core_js_impl.dart (1)

1-11:

Details

✅ Verification successful

Excellent migration to modern Dart JS interop.

This migration from package:js to dart:js_interop is well-implemented and aligns with Dart 3.7+ best practices. The use of @staticInterop provides better type safety compared to the deprecated dynamic interop approach.

Verify that the migration is complete across the entire codebase:


🏁 Script executed:

#!/bin/bash
# Description: Verify complete migration from package:js to dart:js_interop
# Expected: No remaining usage of package:js annotations or imports

echo "Checking for remaining package:js usage..."
echo

# Check for old package:js imports
echo "=== Checking for package:js imports ==="
rg "import 'package:js/" --type dart || echo "✓ No package:js imports found"

# Check for old @JS annotations (should be replaced with dart:js_interop)
echo "=== Checking for @JS() usage without dart:js_interop ==="
rg "@JS\(" --type dart -A 2 -B 2

# Check for dart:js_interop imports to verify new implementation
echo "=== Checking dart:js_interop imports ==="
rg "import 'dart:js_interop'" --type dart || echo "No dart:js_interop imports found"

# Check for @staticInterop usage
echo "=== Checking @staticInterop usage ==="
rg "@staticInterop" --type dart || echo "No @staticInterop annotations found"

Length of output: 11147


✅ Complete migration to dart:js_interop confirmed

All package:js imports have been removed, and every JS interop class now imports dart:js_interop. Your @JS annotations reference the correct package, and @staticInterop is applied consistently throughout the codebase. Great job!

CHANGELOG.md (1)

1-10: Comprehensive migration documentation.

The changelog effectively documents the migration from dart:js to dart:js_interop. The listed changes are comprehensive and accurately reflect the scope of modernizing Office.js interop for type safety and future compatibility.

lib/src/office/office.dart (3)

1-1: Correct import migration to dart:js_interop.

The import has been properly updated from the deprecated package:js/js.dart to the modern dart:js_interop.


51-52: Factory constructor expressions are correctly simplified.

The factory constructors have been appropriately refactored to use concise expression body syntax while maintaining the same functionality with the expando pattern.

Also applies to: 68-69, 89-90


97-97: Proper JS string conversion to Dart.

The .toDart conversion is correctly applied to convert the JS string to a Dart string before enum lookup.

.cursor/rules/flutter_ui_dev.mdc (1)

1-59: Well-structured Flutter development guidelines.

The guidelines provide comprehensive and thoughtful recommendations for Flutter UI development. Key strengths:

  • Clear preference hierarchy for code organization (comments → methods → widgets)
  • Practical decision tree for widget extraction
  • Performance considerations balanced with maintainability
  • Emphasis on documenting rationale rather than implementation details

The guidelines promote sensible practices that will help maintain clean, performant Flutter code.

lib/src/office_interops/office_js_impl.dart (3)

1-1: Correct import for modern JS interop.

The import has been properly updated to use dart:js_interop instead of the deprecated package.


12-14: Proper static interop pattern implementation.

The migration to @staticInterop classes with full namespace annotations ('Office.Context', 'Office.Mailbox', 'Office.Item') follows the correct modern pattern for Dart JS interop.

Also applies to: 52-54, 94-96


16-34: Correct extension method pattern and type usage.

The extension methods properly expose the JS properties and methods. The use of JSString? instead of String? for the itemType property is appropriate for JS interop type safety.

Also applies to: 56-69, 98-110

.cursor/rules/dart_dev.mdc (1)

1-72: Comprehensive Dart development guidelines aligned with the PR objectives!

The rules effectively support the migration to Dart 3.7 and establish clear standards for:

  • Modern Dart features (null safety, pattern matching)
  • Enhanced documentation with dartdoc templates
  • Coding conventions and best practices
  • Project structure alignment

The guidelines will help maintain consistency across the codebase during and after the migration.

lib/src/js_interops/office_helpers_js_impl.dart (1)

1-56: Excellent migration to the new JS interop model!

The migration correctly implements the modern dart:js_interop patterns:

  • ✅ Proper use of @staticInterop with extension methods
  • ✅ Correct JSFunction type for callback parameters
  • ✅ Appropriate JSAny? return type for dynamic values
  • ✅ Clean separation of external declarations

This aligns perfectly with the PR objective to migrate from deprecated interop to Dart 3.7's static interop API.

lib/src/utils/interop_utils.dart (2)

46-51: Excellent error handling for function conversion!

The explicit error message guides developers to use typed functions with .toJS extension methods instead of attempting automatic conversion. This promotes type safety and aligns with the new JS interop model.


88-124: Well-implemented dartify function with comprehensive type handling!

The dartify implementation correctly handles all common JS types and provides a clean inverse of the jsify function. The recursive handling of arrays and objects ensures proper deep conversion.

PLAN.md (1)

17-191: Comprehensive and well-structured migration plan!

The migration instructions are thorough and provide excellent guidance for migrating from the deprecated package:js to the modern dart:js_interop API. The examples and step-by-step approach will be very helpful for implementers.

🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

49-49: Spaces inside code span elements
null

(MD038, no-space-in-code)

lib/src/js_interops/es6_js_impl.dart (1)

1-52: Excellent migration to dart:js_interop!

The conversion from the deprecated package:js to the modern dart:js_interop API is implemented correctly:

  • Proper use of @staticInterop with extension types
  • Correct JS type annotations (JSFunction, JSNumber, JSArray, JSString)
  • Static methods appropriately converted to top-level external functions
  • Clean and consistent implementation throughout
lib/src/office/office_extension.dart (3)

17-17: Clean factory constructor refactor!

The simplified expression body makes the code more concise and readable.


24-28: Correct conversion to JS types!

The conversion from Dart List<String> to JSArray<JSString> is properly implemented using .toJS on individual strings and the list.


52-52: Proper boolean conversion!

Using .toDart to convert JSBoolean to Dart bool is the correct approach.

lib/src/office_interops/office_extension_js_impl.dart (1)

1-141: Excellent migration to dart:js_interop!

The migration from the deprecated package:js to the modern dart:js_interop static interop approach is well-executed. All JS interop types are properly used, and the @staticInterop pattern with extension methods is correctly implemented throughout the file.

lib/src/office_interops/excel_js_impl.dart (2)

1-439: Well-structured migration to dart:js_interop!

The migration is comprehensive and consistent. All Dart native types have been properly replaced with their JS interop counterparts (JSString, JSNumber, JSBoolean, JSArray). The @staticInterop pattern with extension methods is correctly applied throughout.


389-396: Verify the getUsedRange signature change

The getUsedRange method signature has changed to require a JSBoolean? parameter instead of being optional. This appears to be a breaking change that could impact existing code.

Please verify if this signature change is intentional and aligns with the Excel API requirements. If the parameter should be optional, consider making it optional in the external declaration.

lib/src/office/excel.dart (1)

16-32: Clean implementation of Excel.run()

The refactored Excel.run() method properly uses .toJS for function conversion, which aligns well with the new JS interop model. The async handling with JSPromise is correctly implemented.

Comment on lines +17 to 24
JSFunction createCallback() =>
((final JSAny? info) {
// Return the info directly
}).toJS;

final promise = officeHelper.officeOnReady(createCallback());
final jsInfoJsonJS = await handleThenable(promise as JSPromise<JSAny?>);
if (jsInfoJsonJS == null) return null;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Fix callback implementation and promise handling.

The callback function implementation has issues:

  1. The callback currently doesn't use the info parameter - it should return the info
  2. The promise handling pattern needs verification

Apply this diff to fix the callback implementation:

-    JSFunction createCallback() =>
-        ((final JSAny? info) {
-          // Return the info directly
-        }).toJS;
+    JSFunction createCallback() =>
+        ((final JSAny? info) => info).toJS;

Let me verify the promise handling pattern used here:


🏁 Script executed:

#!/bin/bash
# Description: Verify handleThenable usage pattern in interop utilities
# Expected: Find handleThenable function definition and usage examples

ast-grep --pattern 'handleThenable($_)'

Length of output: 41


Let's locate the handleThenable definition and its signature for proper promise handling:


🏁 Script executed:

#!/bin/bash
# Search for handleThenable definition and nearby context
rg "handleThenable" -n -C3

Length of output: 2811


Fix callback to return the info parameter

The promise handling via handleThenable matches the established interop pattern and requires no changes. However, the JS callback must return its info argument:

• lib/src/office/office.dart (around lines 17–24): update createCallback to return info.

-    JSFunction createCallback() =>
-        ((final JSAny? info) {
-          // Return the info directly
-        }).toJS;
+    JSFunction createCallback() =>
+        ((final JSAny? info) => info).toJS;
📝 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.

Suggested change
JSFunction createCallback() =>
((final JSAny? info) {
// Return the info directly
}).toJS;
final promise = officeHelper.officeOnReady(createCallback());
final jsInfoJsonJS = await handleThenable(promise as JSPromise<JSAny?>);
if (jsInfoJsonJS == null) return null;
JSFunction createCallback() =>
((final JSAny? info) => info).toJS;
final promise = officeHelper.officeOnReady(createCallback());
final jsInfoJsonJS = await handleThenable(promise as JSPromise<JSAny?>);
if (jsInfoJsonJS == null) return null;
🤖 Prompt for AI Agents
In lib/src/office/office.dart around lines 17 to 24, the callback function
created by createCallback does not return the info parameter it receives, which
is necessary for correct behavior. Modify the callback implementation so that it
explicitly returns the info argument. The promise handling with handleThenable
is correct and does not need changes.

Comment on lines 30 to 40
Future<T> sync<T>([final T? passThroughValue]) async {
return handleThenable(jsObject.sync(passThroughValue));
if (passThroughValue is JSAny?) {
final jsResult = jsObject.sync<JSAny?>(passThroughValue);
final result = await handleThenable(jsResult);
return result as T;
} else {
final jsResult = jsObject.sync<JSAny?>();
final result = await handleThenable(jsResult);
return passThroughValue ?? result as T;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Consider adding type safety checks for the generic cast.

The type casts at lines 34 and 38 (as T) could throw at runtime if T is not compatible with the actual return type. Consider adding runtime type checks or documenting the expected types.


🏁 Script executed:

#!/bin/bash
# Search for usages of sync method to understand what types are typically used
rg -A 5 "\.sync<" --type dart

Length of output: 716


Add runtime type validation for the generic cast

The unchecked casts on lines 34 and 38 (result as T) can fail at runtime if T doesn’t match the actual type returned by handleThenable. To prevent unexpected errors:

• Location:

  • lib/src/office/office_extension.dart
    • Line 34: return result as T;
    • Line 38: return passThroughValue ?? result as T;

• Suggested fixes:

  • Add an assert or runtime check before casting, e.g.
    if (result is! T) {
      throw StateError('Expected result of type $T, got ${result.runtimeType}');
    }
    return result as T;
  • Or wrap the cast in a try/catch and provide a clear error message.
  • Document in method comments which types callers can safely expect.
🤖 Prompt for AI Agents
In lib/src/office/office_extension.dart around lines 30 to 40, the generic casts
using 'as T' on lines 34 and 38 can cause runtime errors if the actual type does
not match T. To fix this, add runtime type checks before casting by verifying if
the result is of type T; if not, throw a clear StateError indicating the
expected and actual types. Alternatively, wrap the cast in try/catch to provide
informative error messages. Also, update the method documentation to specify the
expected return types to callers.

Comment thread lib/src/office/excel.dart
Comment on lines +271 to +279
List<List<dynamic>> get numberFormat {
final jsValues = jsObject.values;
return (dartify(jsValues)! as List).cast<List<dynamic>>();
}

set numberFormat(final List<List<dynamic>> values) {
final jsValues = jsify(values);
jsObject.values = jsValues!;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: numberFormat property incorrectly uses values property

The numberFormat getter and setter are accessing jsObject.values instead of a numberFormat property. This will cause the numberFormat property to overwrite the cell values.

The numberFormat property should access its own corresponding JS property. Based on the Excel API, this should likely be:

  List<List<dynamic>> get numberFormat {
-   final jsValues = jsObject.values;
+   final jsValues = jsObject.numberFormat;
    return (dartify(jsValues)! as List).cast<List<dynamic>>();
  }

  set numberFormat(final List<List<dynamic>> values) {
    final jsValues = jsify(values);
-   jsObject.values = jsValues!;
+   jsObject.numberFormat = jsValues!;
  }

However, this requires that the numberFormat property is properly exposed in the RangeJsImpl extension in excel_js_impl.dart.

📝 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.

Suggested change
List<List<dynamic>> get numberFormat {
final jsValues = jsObject.values;
return (dartify(jsValues)! as List).cast<List<dynamic>>();
}
set numberFormat(final List<List<dynamic>> values) {
final jsValues = jsify(values);
jsObject.values = jsValues!;
}
List<List<dynamic>> get numberFormat {
final jsValues = jsObject.numberFormat;
return (dartify(jsValues)! as List).cast<List<dynamic>>();
}
set numberFormat(final List<List<dynamic>> values) {
final jsValues = jsify(values);
jsObject.numberFormat = jsValues!;
}
🤖 Prompt for AI Agents
In lib/src/office/excel.dart around lines 271 to 279, the numberFormat getter
and setter incorrectly access jsObject.values instead of the numberFormat
property, causing overwriting of cell values. Update both getter and setter to
access jsObject.numberFormat instead of jsObject.values. Also, ensure that the
numberFormat property is properly defined and exposed in the RangeJsImpl
extension within excel_js_impl.dart to support this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adapt new js_interop API

1 participant