fix: support include keyword in thrift ts generation #178
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the frontend's Thrift-related type definitions and code generation process. The changes involve removing multiple Thrift-generated TypeScript files, introducing new type declaration files, and modifying the code generation script to use a shell-based approach instead of direct Thrift TypeScript generation. Changes
Possibly related PRs
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
frontend/src/lib/types/codegen/orchestration_types.d.ts (2)
66-71: Add more detailed JSDoc comments.The current documentation for TabularData could be more descriptive about the purpose of each field.
108-119: Enhance documentation for NodeConnections.The multi-pass explanation could benefit from clearer examples of how the hashes are used.
.github/workflows/test_frontend.yaml (1)
36-39: Optimize apt-get commands.Combine update and install commands to reduce layers and add -qq flag for quieter output.
- sudo apt-get update - sudo apt-get install -y thrift-compiler + sudo apt-get update -qq && sudo apt-get install -qq -y thrift-compiler
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (48)
.github/workflows/test_frontend.yaml(4 hunks)frontend/package.json(2 hunks)frontend/scripts/codegen.sh(1 hunks)frontend/src/lib/types/codegen/ai/chronon/api/Accuracy.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/Aggregation.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/AggregationPart.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/BootstrapPart.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/Cardinality.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/DataField.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/DataKind.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/DataSpec.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/Derivation.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/DriftMetric.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/DriftSpec.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/EntitySource.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/EventSource.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/ExternalPart.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/ExternalSource.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/GroupBy.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/GroupByServingInfo.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/Join.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/JoinPart.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/JoinSource.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/LabelPart.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/MetaData.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/Model.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/ModelType.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/Operation.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/Query.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/Source.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/StagingQuery.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/TDataType.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/TileDrift.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/TileDriftSeries.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/TileKey.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/TileSeriesKey.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/TileSummary.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/TileSummarySeries.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/TimeUnit.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/Window.ts(0 hunks)frontend/src/lib/types/codegen/ai/chronon/api/index.ts(0 hunks)frontend/src/lib/types/codegen/api_types.d.ts(1 hunks)frontend/src/lib/types/codegen/common_types.d.ts(1 hunks)frontend/src/lib/types/codegen/common_types.js(1 hunks)frontend/src/lib/types/codegen/observability_types.d.ts(1 hunks)frontend/src/lib/types/codegen/observability_types.js(1 hunks)frontend/src/lib/types/codegen/orchestration_types.d.ts(1 hunks)frontend/src/lib/types/codegen/orchestration_types.js(1 hunks)
💤 Files with no reviewable changes (38)
- frontend/src/lib/types/codegen/ai/chronon/api/DataKind.ts
- frontend/src/lib/types/codegen/ai/chronon/api/Accuracy.ts
- frontend/src/lib/types/codegen/ai/chronon/api/Cardinality.ts
- frontend/src/lib/types/codegen/ai/chronon/api/ModelType.ts
- frontend/src/lib/types/codegen/ai/chronon/api/JoinPart.ts
- frontend/src/lib/types/codegen/ai/chronon/api/LabelPart.ts
- frontend/src/lib/types/codegen/ai/chronon/api/DataSpec.ts
- frontend/src/lib/types/codegen/ai/chronon/api/Query.ts
- frontend/src/lib/types/codegen/ai/chronon/api/DriftMetric.ts
- frontend/src/lib/types/codegen/ai/chronon/api/TDataType.ts
- frontend/src/lib/types/codegen/ai/chronon/api/GroupBy.ts
- frontend/src/lib/types/codegen/ai/chronon/api/GroupByServingInfo.ts
- frontend/src/lib/types/codegen/ai/chronon/api/Join.ts
- frontend/src/lib/types/codegen/ai/chronon/api/TileKey.ts
- frontend/src/lib/types/codegen/ai/chronon/api/StagingQuery.ts
- frontend/src/lib/types/codegen/ai/chronon/api/AggregationPart.ts
- frontend/src/lib/types/codegen/ai/chronon/api/Derivation.ts
- frontend/src/lib/types/codegen/ai/chronon/api/Operation.ts
- frontend/src/lib/types/codegen/ai/chronon/api/TimeUnit.ts
- frontend/src/lib/types/codegen/ai/chronon/api/JoinSource.ts
- frontend/src/lib/types/codegen/ai/chronon/api/EntitySource.ts
- frontend/src/lib/types/codegen/ai/chronon/api/TileSummary.ts
- frontend/src/lib/types/codegen/ai/chronon/api/EventSource.ts
- frontend/src/lib/types/codegen/ai/chronon/api/Model.ts
- frontend/src/lib/types/codegen/ai/chronon/api/Aggregation.ts
- frontend/src/lib/types/codegen/ai/chronon/api/MetaData.ts
- frontend/src/lib/types/codegen/ai/chronon/api/DataField.ts
- frontend/src/lib/types/codegen/ai/chronon/api/BootstrapPart.ts
- frontend/src/lib/types/codegen/ai/chronon/api/TileDriftSeries.ts
- frontend/src/lib/types/codegen/ai/chronon/api/ExternalSource.ts
- frontend/src/lib/types/codegen/ai/chronon/api/ExternalPart.ts
- frontend/src/lib/types/codegen/ai/chronon/api/DriftSpec.ts
- frontend/src/lib/types/codegen/ai/chronon/api/TileSeriesKey.ts
- frontend/src/lib/types/codegen/ai/chronon/api/Source.ts
- frontend/src/lib/types/codegen/ai/chronon/api/TileDrift.ts
- frontend/src/lib/types/codegen/ai/chronon/api/TileSummarySeries.ts
- frontend/src/lib/types/codegen/ai/chronon/api/Window.ts
- frontend/src/lib/types/codegen/ai/chronon/api/index.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/src/lib/types/codegen/common_types.d.ts
- frontend/src/lib/types/codegen/observability_types.d.ts
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/src/lib/types/codegen/common_types.js
[error] 6-6: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 18-19: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
frontend/src/lib/types/codegen/orchestration_types.js
[error] 565-565: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 577-577: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 676-676: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 689-689: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1060-1060: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1160-1160: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1889-1889: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 6-6: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
frontend/src/lib/types/codegen/observability_types.js
[error] 277-277: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 289-289: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 322-322: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 334-334: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 694-694: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 698-698: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 713-713: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 718-718: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 733-733: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 745-745: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 757-757: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 769-769: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 781-781: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 785-785: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 800-800: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 804-804: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 819-819: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1209-1209: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1221-1221: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1233-1233: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1245-1245: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1257-1257: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1269-1269: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1281-1281: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1293-1293: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1305-1305: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1467-1467: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1479-1479: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1492-1492: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 1510-1510: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 6-6: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🔇 Additional comments (4)
frontend/src/lib/types/codegen/api_types.d.ts (1)
1-309: LGTM!The type definitions are comprehensive and correctly structured.
frontend/src/lib/types/codegen/common_types.js (1)
18-19:⚠️ Potential issueAvoid Reassigning Global Variables
Declare
TimeUnitusingconst,let, orvarto prevent overriding global variables.Apply this diff:
- TimeUnit = { + const TimeUnit = {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 18-19: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
frontend/src/lib/types/codegen/orchestration_types.js (1)
565-565:⚠️ Potential issueReplace hasOwnProperty with Object.hasOwn().
Using hasOwnProperty directly is unsafe as it can be overridden. Use Object.hasOwn() instead.
-if (this.parents.hasOwnProperty(iter8)) +if (Object.hasOwn(this.parents, iter8))Also applies to: 577-577, 676-676, 689-689, 1060-1060, 1160-1160, 1889-1889
🧰 Tools
🪛 Biome (1.9.4)
[error] 565-565: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
frontend/package.json (1)
68-68: Consider updating tailwind-variantsCurrent: 0.3.0, Latest may have improvements.
| output.writeFieldBegin('percentiles', Thrift.Type.LIST, 1); | ||
| output.writeListBegin(Thrift.Type.DOUBLE, this.percentiles.length); | ||
| for (var iter17 in this.percentiles) { | ||
| if (this.percentiles.hasOwnProperty(iter17)) { |
There was a problem hiding this comment.
Use Object.hasOwn() Instead of hasOwnProperty
Replace hasOwnProperty calls with Object.hasOwn() to prevent potential prototype pollution issues.
Apply this diff:
- if (this.percentiles.hasOwnProperty(iter17)) {
+ if (Object.hasOwn(this.percentiles, iter17)) {📝 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 (this.percentiles.hasOwnProperty(iter17)) { | |
| if (Object.hasOwn(this.percentiles, iter17)) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 277-277: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
| output.writeFieldBegin('histogram', Thrift.Type.MAP, 2); | ||
| output.writeMapBegin(Thrift.Type.STRING, Thrift.Type.I64, Thrift.objectLength(this.histogram)); | ||
| for (var kiter18 in this.histogram) { | ||
| if (this.histogram.hasOwnProperty(kiter18)) { |
There was a problem hiding this comment.
Use Object.hasOwn() Instead of hasOwnProperty
Replace this.histogram.hasOwnProperty(kiter18) with Object.hasOwn(this.histogram, kiter18).
Apply this diff:
- if (this.histogram.hasOwnProperty(kiter18)) {
+ if (Object.hasOwn(this.histogram, kiter18)) {📝 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 (this.histogram.hasOwnProperty(kiter18)) { | |
| if (Object.hasOwn(this.histogram, kiter18)) { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 289-289: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
| // | ||
| // DO NOT EDIT UNLESS YOU ARE SURE THAT YOU KNOW WHAT YOU ARE DOING | ||
| // | ||
| if (typeof Int64 === 'undefined' && typeof require === 'function') { |
There was a problem hiding this comment.
Declare Int64 Before Use
Int64 is used before it is declared. Initialize it before the condition.
Apply this diff:
- if (typeof Int64 === 'undefined' && typeof require === 'function') {
+ var Int64;
+ if (typeof Int64 === 'undefined' && typeof require === 'function') {📝 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 (typeof Int64 === 'undefined' && typeof require === 'function') { | |
| var Int64; | |
| if (typeof Int64 === 'undefined' && typeof require === 'function') { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
| #!/bin/bash | ||
|
|
||
| THRIFT_DIR="../api/thrift" | ||
| OUT_DIR="src/lib/types/codegen" | ||
|
|
||
| mkdir -p $OUT_DIR | ||
|
|
||
| for file in $THRIFT_DIR/*.thrift; do | ||
| thrift -r -out $OUT_DIR --gen js:ts "$file" | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and validation.
The script needs:
- Error checking for thrift command
- Path validation
- Exit on error
#!/bin/bash
+set -euo pipefail
THRIFT_DIR="../api/thrift"
OUT_DIR="src/lib/types/codegen"
+if ! command -v thrift &> /dev/null; then
+ echo "Error: thrift compiler not found"
+ exit 1
+fi
+
+if [ ! -d "$THRIFT_DIR" ]; then
+ echo "Error: Thrift directory not found"
+ exit 1
+fi
+
mkdir -p $OUT_DIR
for file in $THRIFT_DIR/*.thrift; do
thrift -r -out $OUT_DIR --gen js:ts "$file"
done
+📝 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.
| #!/bin/bash | |
| THRIFT_DIR="../api/thrift" | |
| OUT_DIR="src/lib/types/codegen" | |
| mkdir -p $OUT_DIR | |
| for file in $THRIFT_DIR/*.thrift; do | |
| thrift -r -out $OUT_DIR --gen js:ts "$file" | |
| done | |
| #!/bin/bash | |
| set -euo pipefail | |
| THRIFT_DIR="../api/thrift" | |
| OUT_DIR="src/lib/types/codegen" | |
| if ! command -v thrift &> /dev/null; then | |
| echo "Error: thrift compiler not found" | |
| exit 1 | |
| fi | |
| if [ ! -d "$THRIFT_DIR" ]; then | |
| echo "Error: Thrift directory not found" | |
| exit 1 | |
| fi | |
| mkdir -p $OUT_DIR | |
| for file in $THRIFT_DIR/*.thrift; do | |
| thrift -r -out $OUT_DIR --gen js:ts "$file" | |
| done |
| @@ -6,7 +6,7 @@ | |||
| "prepare": "npm run codegen", | |||
| "dev": "npm run codegen && vite dev", | |||
| "build": "npm run codegen && vite build", | |||
| "codegen": "thrift-typescript --target thrift-server --rootDir .. --sourceDir api/thrift --outDir frontend/src/lib/types/codegen api.thrift", | |||
| "codegen": "bash scripts/codegen.sh", | |||
There was a problem hiding this comment.
💡 Codebase verification
Fix codegen script permissions and implementation
- Script needs executable permissions (
chmod +x) - Relative paths may break; use
$(dirname "$0")/..for reliable paths - Thrift CLI tool dependency needs to be documented
🔗 Analysis chain
Verify codegen shell script existence
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the codegen script exists and is executable
if [ -f "frontend/scripts/codegen.sh" ]; then
ls -l frontend/scripts/codegen.sh
else
echo "Error: codegen.sh not found"
exit 1
fi
Length of output: 152
Script:
#!/bin/bash
# View codegen script content
cat frontend/scripts/codegen.sh
Length of output: 216
ken-zlai
changed the title
## Summary This PR removes the `thrift-typescript` package and its usage, as Thrift supports TypeScript generation natively. Using Thrift's built-in TypeScript support simplifies our workflow and eliminates the need for the additional dependency, as well as out of the box support for the `include` keyword. ### Changes - Removed `thrift-typescript` packages from the project. - Replaced the existing setup with Thrift's native TypeScript generation. - Updated the Thrift configuration and build process to align with the new approach. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit Based on the comprehensive changes, here are the release notes: - **Code Generation** - Replaced Thrift-based code generation with a new approach - Introduced new TypeScript declaration files for API, common, observability, and orchestration types - Removed previous Thrift-generated TypeScript files - **Dependency Management** - Removed Thrift-related dependencies from `package.json` - Updated code generation script to use a custom shell script - **Development Workflow** - Updated ESLint configuration to ignore generated type files - Simplified type generation process <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Thomas Chow <[email protected]>
## Summary This PR removes the `thrift-typescript` package and its usage, as Thrift supports TypeScript generation natively. Using Thrift's built-in TypeScript support simplifies our workflow and eliminates the need for the additional dependency, as well as out of the box support for the `include` keyword. ### Changes - Removed `thrift-typescript` packages from the project. - Replaced the existing setup with Thrift's native TypeScript generation. - Updated the Thrift configuration and build process to align with the new approach. ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit Based on the comprehensive changes, here are the release notes: - **Code Generation** - Replaced Thrift-based code generation with a new approach - Introduced new TypeScript declaration files for API, common, observability, and orchestration types - Removed previous Thrift-generated TypeScript files - **Dependency Management** - Removed Thrift-related dependencies from `package.json` - Updated code generation script to use a custom shell script - **Development Workflow** - Updated ESLint configuration to ignore generated type files - Simplified type generation process <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
This PR removes the
thrift-typescriptpackage and its usage, as Thrift supports TypeScript generation natively. Using Thrift's built-in TypeScript support simplifies our workflow and eliminates the need for the additional dependency, as well as out of the box support for theincludekeyword.Changes
thrift-typescriptpackages from the project.Checklist
Summary by CodeRabbit
Based on the comprehensive changes, here are the release notes:
Code Generation
Dependency Management
package.jsonDevelopment Workflow