Open
Conversation
…lestone progress visualization
…d milestone progress calculation hook
…for milestone visualization
…rowMilestoneProgressDonut component
📝 WalkthroughWalkthroughAdds a new Escrow Milestone Progress feature: types, calculation utilities, a React hook, bar and donut visualization components, a demo page with scenarios, and README documentation. Demo contains a stray inline comment in a scenario description. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
JoelVR17
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/components/tw-blocks/escrows/indicators/milestone-progress/bar/EscrowMilestoneProgress.tsx (1)
37-59: Consider extracting shared status-state UI between bar and donut.The invalid/empty-state rendering here is very similar to
src/components/tw-blocks/escrows/indicators/milestone-progress/donut/EscrowMilestoneProgress.tsx. A small shared helper/component would reduce divergence risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tw-blocks/escrows/indicators/milestone-progress/bar/EscrowMilestoneProgress.tsx` around lines 37 - 59, Extract the repeated invalid/empty-state UI into a small shared component (e.g., MilestoneStatus or MilestoneStatusMessage) and use it in both EscrowMilestoneProgress (bar) and the donut EscrowMilestoneProgress; move the JSX that handles !isValid (shows errorMessage) and totalMilestones === 0 (shows "No milestones" + Progress value=0) into that shared component, expose props for className, isValid, errorMessage, totalMilestones (or separate flags like showEmpty and showError), then replace the two inline blocks in the bar component with calls to the new shared component and import the same component into the donut version so both render the exact same UI.src/components/tw-blocks/escrows/indicators/milestone-progress/index.ts (1)
32-33: Prefer exporting components via their submodule barrels.Using
./barand./donuthere avoids hard-coding deeper file paths and keeps the public surface easier to maintain.Suggested refactor
-export { EscrowMilestoneProgressBar } from "./bar/EscrowMilestoneProgress"; -export { EscrowMilestoneProgressDonut } from "./donut/EscrowMilestoneProgress"; +export { EscrowMilestoneProgressBar } from "./bar"; +export { EscrowMilestoneProgressDonut } from "./donut";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tw-blocks/escrows/indicators/milestone-progress/index.ts` around lines 32 - 33, The exports currently reference deep files EscrowMilestoneProgressBar and EscrowMilestoneProgressDonut directly; change them to re-export from the submodule barrels instead (use "./bar" and "./donut" module barrels that should export EscrowMilestoneProgressBar and EscrowMilestoneProgressDonut) so the public surface isn't tied to internal file paths, and ensure the barrels in bar/index.ts and donut/index.ts export those component symbols.src/app/demo/milestone-progress/page.tsx (1)
106-110:{} as Escrowspread silently omits all requiredEscrowfields.The spread produces a runtime object with only
typeandmilestonesset; every otherEscrowfield isundefined. IfuseEscrowMilestoneProgressor the progress components access any additional field, they'll encounter silentundefinedrather than a type error. For a demo page this is low-risk, but it's worth either adding a// demo-only: partial mockcomment or constructing the object more explicitly.♻️ Suggested approach — explicit Partial cast with comment
- const mockEscrow = { - ...({} as Escrow), - type: scenario.type as "single-release" | "multi-release", - milestones: scenario.milestones, - }; + // Demo-only: only the fields consumed by milestone-progress components are populated. + const mockEscrow = { + type: scenario.type as "single-release" | "multi-release", + milestones: scenario.milestones, + } as Escrow;This removes the misleading spread while making the intentional incompleteness visible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/demo/milestone-progress/page.tsx` around lines 106 - 110, The mockEscrow creation currently uses "{} as Escrow" which silently omits required fields; update the demo to make the incompleteness explicit by either (a) replacing "{} as Escrow" with a Partial<Escrow> cast (e.g., const mockEscrow: Partial<Escrow> = { type: ..., milestones: ... }) and add a "// demo-only: partial mock" comment, or (b) explicitly construct mockEscrow with all fields required by Escrow; ensure callers like useEscrowMilestoneProgress and any progress components see the correct type (or handle Partial) so no required field is silently undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/demo/milestone-progress/page.tsx`:
- Line 361: The <label> elements used for decorative text (e.g., the element
rendering "Aprobados" and the other similar label at the same block) are not
associated with form controls and should be replaced with a semantically neutral
element; update those occurrences in milestone-progress/page.tsx to use a <span>
or <p> (preserving the className "text-sm font-medium") instead of <label> so
they no longer imply form semantics.
- Line 271: The CardDescription nodes are using the JSX expression {false},
which React renders as nothing; update the two occurrences inside
src/app/demo/milestone-progress/page.tsx (the CardDescription that currently
reads "Opción showText={false}" and the other "showText={false}") so they render
the literal text rather than a boolean: replace the JSX boolean expression with
a string literal containing the braces and false (e.g., the characters
"{false}") or wrap the literal in a <code> element for semantic rendering;
ensure you update both CardDescription usages so the prop syntax appears in the
UI.
In
`@src/components/tw-blocks/escrows/indicators/milestone-progress/donut/EscrowMilestoneProgress.tsx`:
- Around line 60-62: The color threshold logic in EscrowMilestoneProgress.tsx
misclassifies percentages exactly equal to 33 and 66; update the comparisons
that use the percentage variable so the ranges are inclusive (e.g., treat 33 as
low and 66 as mid) by changing the first two checks from strict < to <= so the
buckets become 1–33, 34–66, 67–100 while keeping the same return values.
In `@src/components/tw-blocks/escrows/indicators/milestone-progress/README.md`:
- Line 181: Update the accessibility bullet that currently reads "Screen reader
friendly text labels" to use the hyphenated compound adjective
"Screen-reader-friendly text labels" in the README entry for milestone-progress
indicators so the phrase is consistent and accessible; find the exact string
"Screen reader friendly text labels" and replace it with "Screen-reader-friendly
text labels".
- Around line 86-88: The fenced code block containing the expression "Progress %
= (completed_milestones / total_milestones) × 100" is missing a language
identifier and triggers markdownlint MD040; update the fence to include a
language (e.g., change ``` to ```text or ```plain) so the block becomes ```text
... ``` to satisfy the linter.
In
`@src/components/tw-blocks/escrows/indicators/milestone-progress/useEscrowMilestoneProgress.ts`:
- Around line 72-94: The zero-milestone early return in
useEscrowMilestoneProgress currently returns isValid: true before the mode/type
compatibility check, allowing an invalid combination (mode === "released" with
escrow.type !== "multi-release") to be marked valid; update the logic in
useEscrowMilestoneProgress so the validation for mode and escrow.type (the
"'released' mode only works with multi-release escrows" check) runs before or as
part of handling totalMilestones === 0 (i.e., perform the mode/type
compatibility check using mode and escrow.type even when totalMilestones is 0
and return isValid: false with the same errorMessage if it fails).
---
Nitpick comments:
In `@src/app/demo/milestone-progress/page.tsx`:
- Around line 106-110: The mockEscrow creation currently uses "{} as Escrow"
which silently omits required fields; update the demo to make the incompleteness
explicit by either (a) replacing "{} as Escrow" with a Partial<Escrow> cast
(e.g., const mockEscrow: Partial<Escrow> = { type: ..., milestones: ... }) and
add a "// demo-only: partial mock" comment, or (b) explicitly construct
mockEscrow with all fields required by Escrow; ensure callers like
useEscrowMilestoneProgress and any progress components see the correct type (or
handle Partial) so no required field is silently undefined.
In
`@src/components/tw-blocks/escrows/indicators/milestone-progress/bar/EscrowMilestoneProgress.tsx`:
- Around line 37-59: Extract the repeated invalid/empty-state UI into a small
shared component (e.g., MilestoneStatus or MilestoneStatusMessage) and use it in
both EscrowMilestoneProgress (bar) and the donut EscrowMilestoneProgress; move
the JSX that handles !isValid (shows errorMessage) and totalMilestones === 0
(shows "No milestones" + Progress value=0) into that shared component, expose
props for className, isValid, errorMessage, totalMilestones (or separate flags
like showEmpty and showError), then replace the two inline blocks in the bar
component with calls to the new shared component and import the same component
into the donut version so both render the exact same UI.
In `@src/components/tw-blocks/escrows/indicators/milestone-progress/index.ts`:
- Around line 32-33: The exports currently reference deep files
EscrowMilestoneProgressBar and EscrowMilestoneProgressDonut directly; change
them to re-export from the submodule barrels instead (use "./bar" and "./donut"
module barrels that should export EscrowMilestoneProgressBar and
EscrowMilestoneProgressDonut) so the public surface isn't tied to internal file
paths, and ensure the barrels in bar/index.ts and donut/index.ts export those
component symbols.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
src/app/demo/milestone-progress/page.tsxsrc/components/tw-blocks/escrows/indicators/milestone-progress/README.mdsrc/components/tw-blocks/escrows/indicators/milestone-progress/bar/EscrowMilestoneProgress.tsxsrc/components/tw-blocks/escrows/indicators/milestone-progress/bar/index.tssrc/components/tw-blocks/escrows/indicators/milestone-progress/donut/EscrowMilestoneProgress.tsxsrc/components/tw-blocks/escrows/indicators/milestone-progress/donut/index.tssrc/components/tw-blocks/escrows/indicators/milestone-progress/index.tssrc/components/tw-blocks/escrows/indicators/milestone-progress/types.tssrc/components/tw-blocks/escrows/indicators/milestone-progress/useEscrowMilestoneProgress.ts
| <Card> | ||
| <CardHeader> | ||
| <CardTitle className="text-lg">Barra sin Texto</CardTitle> | ||
| <CardDescription>Opción showText={false}</CardDescription> |
There was a problem hiding this comment.
{false} in JSX silently drops from rendered output, truncating both descriptions.
In JSX, {false} evaluates to the boolean false, which React renders as nothing. Both CardDescription nodes will display only the prefix text — "Opción showText=" (Line 271) and "showText=" (Line 332) — omitting the intended {false} literal.
🐛 Proposed fix — use string literals to show the prop syntax
- <CardDescription>Opción showText={false}</CardDescription>
+ <CardDescription>{`Opción showText={false}`}</CardDescription>- <CardDescription>showText={false}</CardDescription>
+ <CardDescription>{`showText={false}`}</CardDescription>Alternatively, wrap with a <code> element for semantic clarity:
- <CardDescription>Opción showText={false}</CardDescription>
+ <CardDescription>Opción <code>showText={"{false}"}</code></CardDescription>Also applies to: 332-332
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/demo/milestone-progress/page.tsx` at line 271, The CardDescription
nodes are using the JSX expression {false}, which React renders as nothing;
update the two occurrences inside src/app/demo/milestone-progress/page.tsx (the
CardDescription that currently reads "Opción showText={false}" and the other
"showText={false}") so they render the literal text rather than a boolean:
replace the JSX boolean expression with a string literal containing the braces
and false (e.g., the characters "{false}") or wrap the literal in a <code>
element for semantic rendering; ensure you update both CardDescription usages so
the prop syntax appears in the UI.
| <h3 className="font-semibold mb-3">📊 Progreso en Barras</h3> | ||
| <div className="space-y-4"> | ||
| <div> | ||
| <label className="text-sm font-medium">Aprobados</label> |
There was a problem hiding this comment.
<label> without htmlFor — use <p> or <span> for decorative text.
These <label> elements have no associated form control (no htmlFor / no wrapped input). Standalone unassociated <label>s create incorrect ARIA semantics; assistive technologies may attempt to link them to a nearby focusable element. Since the intent is visual text, not a form label, replace with a semantically neutral element.
🛡️ Proposed fix
- <label className="text-sm font-medium">Aprobados</label>
+ <p className="text-sm font-medium">Aprobados</p>- <label className="text-sm font-medium">Liberados</label>
+ <p className="text-sm font-medium">Liberados</p>Also applies to: 371-371
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/demo/milestone-progress/page.tsx` at line 361, The <label> elements
used for decorative text (e.g., the element rendering "Aprobados" and the other
similar label at the same block) are not associated with form controls and
should be replaced with a semantically neutral element; update those occurrences
in milestone-progress/page.tsx to use a <span> or <p> (preserving the className
"text-sm font-medium") instead of <label> so they no longer imply form
semantics.
Comment on lines
+60
to
+62
| if (percentage < 33) return "hsl(var(--destructive))"; | ||
| if (percentage < 66) return "hsl(var(--yellow-500))"; | ||
| return "hsl(var(--primary))"; |
There was a problem hiding this comment.
Progress color thresholds are off by one at 33% and 66%.
With current checks, 33 is not in the low bucket and 66 is not in the mid bucket. If ranges are intended as 1-33, 34-66, 67-100, use inclusive comparisons.
Suggested fix
- if (percentage < 33) return "hsl(var(--destructive))";
- if (percentage < 66) return "hsl(var(--yellow-500))";
+ if (percentage <= 33) return "hsl(var(--destructive))";
+ if (percentage <= 66) return "hsl(var(--yellow-500))";📝 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
| if (percentage < 33) return "hsl(var(--destructive))"; | |
| if (percentage < 66) return "hsl(var(--yellow-500))"; | |
| return "hsl(var(--primary))"; | |
| if (percentage <= 33) return "hsl(var(--destructive))"; | |
| if (percentage <= 66) return "hsl(var(--yellow-500))"; | |
| return "hsl(var(--primary))"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/components/tw-blocks/escrows/indicators/milestone-progress/donut/EscrowMilestoneProgress.tsx`
around lines 60 - 62, The color threshold logic in EscrowMilestoneProgress.tsx
misclassifies percentages exactly equal to 33 and 66; update the comparisons
that use the percentage variable so the ranges are inclusive (e.g., treat 33 as
low and 66 as mid) by changing the first two checks from strict < to <= so the
buckets become 1–33, 34–66, 67–100 while keeping the same return values.
Comment on lines
+86
to
+88
| ``` | ||
| Progress % = (completed_milestones / total_milestones) × 100 | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
At Line 86, the fence is missing a language identifier and triggers markdownlint (MD040).
Suggested fix
-```
+```text
Progress % = (completed_milestones / total_milestones) × 100</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 86-86: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @src/components/tw-blocks/escrows/indicators/milestone-progress/README.md
around lines 86 - 88, The fenced code block containing the expression "Progress
% = (completed_milestones / total_milestones) × 100" is missing a language
identifier and triggers markdownlint MD040; update the fence to include a
language (e.g., change totext or plain) so the block becomes text
... ``` to satisfy the linter.
</details>
<!-- fingerprinting:phantom:medusa:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
| - Proper ARIA labels on progress elements | ||
| - Color contrast meets WCAG AA standards | ||
| - Works with keyboard navigation | ||
| - Screen reader friendly text labels |
There was a problem hiding this comment.
Use hyphenated compound adjective in the accessibility bullet.
At Line 181, use “Screen-reader-friendly text labels”.
Suggested fix
-- Screen reader friendly text labels
+- Screen-reader-friendly text labels📝 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
| - Screen reader friendly text labels | |
| - Screen-reader-friendly text labels |
🧰 Tools
🪛 LanguageTool
[grammar] ~181-~181: Use a hyphen to join words.
Context: ...with keyboard navigation - Screen reader friendly text labels ## Browser Support...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/tw-blocks/escrows/indicators/milestone-progress/README.md` at
line 181, Update the accessibility bullet that currently reads "Screen reader
friendly text labels" to use the hyphenated compound adjective
"Screen-reader-friendly text labels" in the README entry for milestone-progress
indicators so the phrase is consistent and accessible; find the exact string
"Screen reader friendly text labels" and replace it with "Screen-reader-friendly
text labels".
Comment on lines
+72
to
+94
| // Handle zero milestones | ||
| if (totalMilestones === 0) { | ||
| return { | ||
| totalMilestones: 0, | ||
| completedMilestones: 0, | ||
| percentage: 0, | ||
| isValid: true, | ||
| errorMessage: "", | ||
| }; | ||
| } | ||
|
|
||
| // Validation: 'released' mode only works with multi-release escrows | ||
| if (mode === "released" && escrow.type !== "multi-release") { | ||
| return { | ||
| totalMilestones, | ||
| completedMilestones: 0, | ||
| percentage: 0, | ||
| isValid: false, | ||
| errorMessage: | ||
| "'released' mode is only available for multi-release escrows", | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Validate mode/type compatibility before the zero-milestone early return.
At Line 73, the function returns isValid: true before the invalid 'released' + single-release check at Line 84. This incorrectly marks an invalid configuration as valid when there are zero milestones.
Suggested fix
const milestones = escrow.milestones || [];
const totalMilestones = milestones.length;
- // Handle zero milestones
- if (totalMilestones === 0) {
- return {
- totalMilestones: 0,
- completedMilestones: 0,
- percentage: 0,
- isValid: true,
- errorMessage: "",
- };
- }
-
// Validation: 'released' mode only works with multi-release escrows
if (mode === "released" && escrow.type !== "multi-release") {
return {
totalMilestones,
completedMilestones: 0,
percentage: 0,
isValid: false,
errorMessage:
"'released' mode is only available for multi-release escrows",
};
}
+
+ // Handle zero milestones
+ if (totalMilestones === 0) {
+ return {
+ totalMilestones: 0,
+ completedMilestones: 0,
+ percentage: 0,
+ isValid: true,
+ errorMessage: "",
+ };
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/components/tw-blocks/escrows/indicators/milestone-progress/useEscrowMilestoneProgress.ts`
around lines 72 - 94, The zero-milestone early return in
useEscrowMilestoneProgress currently returns isValid: true before the mode/type
compatibility check, allowing an invalid combination (mode === "released" with
escrow.type !== "multi-release") to be marked valid; update the logic in
useEscrowMilestoneProgress so the validation for mode and escrow.type (the
"'released' mode only works with multi-release escrows" check) runs before or as
part of handling totalMilestones === 0 (i.e., perform the mode/type
compatibility check using mode and escrow.type even when totalMilestones is 0
and return isValid: false with the same errorMessage if it fails).
JoelVR17
requested changes
Feb 25, 2026
Contributor
JoelVR17
left a comment
JoelVR17
left a comment
There was a problem hiding this comment.
Great job!
But please review the changes below.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/app/demo/milestone-progress/page.tsx (4)
47-98: Spanish scenario descriptions and UI strings — already flagged for removal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/demo/milestone-progress/page.tsx` around lines 47 - 98, SCENARIOS contains hardcoded Spanish UI strings in the description fields which were flagged for removal; update each description in the SCENARIOS object (e.g., multiReleasePartial, multiRelease100, singleReleasePartial, singleRelease100, empty) to use either English text or, preferably, localization keys/translate calls (e.g., t('milestone.multiRelease.partial')) instead of literal Spanish, ensuring any tests or UI consumers reference the new keys; leave the milestone creators (createMultiReleaseMilestone, createSingleReleaseMilestone) and the structure intact.
361-371: Unassociated<label>elements — already flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/demo/milestone-progress/page.tsx` around lines 361 - 371, The two label elements near the EscrowMilestoneProgressBar usage are not associated with any form control; update them to either be semantic text elements or properly associate them: either replace <label className="...">Aprobados</label> and the adjacent <label>Liberados</label> with non-label elements (e.g., <span> or <div>) if they are purely descriptive, or give them htmlFor attributes and add matching id attributes on the corresponding interactive element (for example on EscrowMilestoneProgressBar or its internal progress element) so the label text is correctly linked to that control (use unique ids like approved-progress and released-progress and set htmlFor="approved-progress"/"released-progress").
332-332:{false}silently drops from JSX — already flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/demo/milestone-progress/page.tsx` at line 332, There is a stray "{false}" expression being rendered in the JSX (near the CardHeader component) which silently drops; locate the JSX in the milestone progress page where CardHeader is used and either remove the "{false}" token entirely or replace it with an explicit conditional/ternary that returns null when you intend to render nothing (e.g., condition ? <Element/> : null), ensuring no dangling "{false}" remains in the CardHeader/parent JSX.
271-271:{false}silently drops from JSX — already flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/demo/milestone-progress/page.tsx` at line 271, The JSX currently contains the literal text "Opción showText={false}" where the "{false}" will be dropped; change this so showText is passed as a real prop to the component instead of part of the children—e.g., update the CardDescription usage to <CardDescription showText={false}>Opción</CardDescription> (or, if the prop belongs to the parent Card, set showText={false} on that component); remove the literal "{false}" from the children so the prop takes effect.
🧹 Nitpick comments (1)
src/app/demo/milestone-progress/page.tsx (1)
106-110:...({} as Escrow)unsafely bypasses TypeScript for all requiredEscrowfields.Spreading
{} as Escrowcasts a fully empty object to theEscrowtype, which silences TS errors for every required field inGetEscrowsFromIndexerResponse. Onlytypeandmilestonesare actually set; any downstream access to other required fields will beundefinedat runtime. For a demo, consider at minimum typingmockEscrowexplicitly so TypeScript still catches mismatches:♻️ Proposed fix
- const mockEscrow = { - ...({} as Escrow), - type: scenario.type as "single-release" | "multi-release", - milestones: scenario.milestones, - }; + const mockEscrow = { + type: scenario.type as "single-release" | "multi-release", + milestones: scenario.milestones, + } as Escrow;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/demo/milestone-progress/page.tsx` around lines 106 - 110, The current mockEscrow spreads an empty object cast to Escrow which silences TypeScript for required fields; instead explicitly type the mock as either Partial<Escrow> (e.g. const mockEscrow: Partial<Escrow> = { type: ..., milestones: ... }) or construct a fully typed Escrow object with all required fields filled from scenario (avoid ...({} as Escrow)). Update the mockEscrow declaration (and any usage of scenario.milestones / scenario.type) so TypeScript will catch missing fields from GetEscrowsFromIndexerResponse rather than allowing undefined at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/demo/milestone-progress/page.tsx`:
- Around line 121-123: Remove the offensive inline comment and the empty
paragraph element in the milestone progress page: delete the comment node {/*
Please eat shit */} and either remove the surrounding <p
className="text-muted-foreground text-lg">...</p> entirely or replace its
contents with the intended descriptive text; locate this in the component
rendered in src/app/demo/milestone-progress/page.tsx where the paragraph with
className "text-muted-foreground text-lg" is defined.
---
Duplicate comments:
In `@src/app/demo/milestone-progress/page.tsx`:
- Around line 47-98: SCENARIOS contains hardcoded Spanish UI strings in the
description fields which were flagged for removal; update each description in
the SCENARIOS object (e.g., multiReleasePartial, multiRelease100,
singleReleasePartial, singleRelease100, empty) to use either English text or,
preferably, localization keys/translate calls (e.g.,
t('milestone.multiRelease.partial')) instead of literal Spanish, ensuring any
tests or UI consumers reference the new keys; leave the milestone creators
(createMultiReleaseMilestone, createSingleReleaseMilestone) and the structure
intact.
- Around line 361-371: The two label elements near the
EscrowMilestoneProgressBar usage are not associated with any form control;
update them to either be semantic text elements or properly associate them:
either replace <label className="...">Aprobados</label> and the adjacent
<label>Liberados</label> with non-label elements (e.g., <span> or <div>) if they
are purely descriptive, or give them htmlFor attributes and add matching id
attributes on the corresponding interactive element (for example on
EscrowMilestoneProgressBar or its internal progress element) so the label text
is correctly linked to that control (use unique ids like approved-progress and
released-progress and set htmlFor="approved-progress"/"released-progress").
- Line 332: There is a stray "{false}" expression being rendered in the JSX
(near the CardHeader component) which silently drops; locate the JSX in the
milestone progress page where CardHeader is used and either remove the "{false}"
token entirely or replace it with an explicit conditional/ternary that returns
null when you intend to render nothing (e.g., condition ? <Element/> : null),
ensuring no dangling "{false}" remains in the CardHeader/parent JSX.
- Line 271: The JSX currently contains the literal text "Opción
showText={false}" where the "{false}" will be dropped; change this so showText
is passed as a real prop to the component instead of part of the children—e.g.,
update the CardDescription usage to <CardDescription
showText={false}>Opción</CardDescription> (or, if the prop belongs to the parent
Card, set showText={false} on that component); remove the literal "{false}" from
the children so the prop takes effect.
---
Nitpick comments:
In `@src/app/demo/milestone-progress/page.tsx`:
- Around line 106-110: The current mockEscrow spreads an empty object cast to
Escrow which silences TypeScript for required fields; instead explicitly type
the mock as either Partial<Escrow> (e.g. const mockEscrow: Partial<Escrow> = {
type: ..., milestones: ... }) or construct a fully typed Escrow object with all
required fields filled from scenario (avoid ...({} as Escrow)). Update the
mockEscrow declaration (and any usage of scenario.milestones / scenario.type) so
TypeScript will catch missing fields from GetEscrowsFromIndexerResponse rather
than allowing undefined at runtime.
Comment on lines
+121
to
+123
| <p className="text-muted-foreground text-lg"> | ||
| {/* Please eat shit */} | ||
| </p> |
There was a problem hiding this comment.
Remove the offensive comment and the resulting empty <p> element.
Line 122 contains {/* Please eat shit */}, an inappropriate comment that must not be merged. After removing it the <p> on lines 121–123 will be empty; either remove the element entirely or add the intended description text.
🐛 Proposed fix
- <p className="text-muted-foreground text-lg">
- {/* Please eat shit */}
- </p>
+ <p className="text-muted-foreground text-lg">
+ Visualización interactiva de hitos de escrow
+ </p>📝 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
| <p className="text-muted-foreground text-lg"> | |
| {/* Please eat shit */} | |
| </p> | |
| <p className="text-muted-foreground text-lg"> | |
| Visualización interactiva de hitos de escrow | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/demo/milestone-progress/page.tsx` around lines 121 - 123, Remove the
offensive inline comment and the empty paragraph element in the milestone
progress page: delete the comment node {/* Please eat shit */} and either remove
the surrounding <p className="text-muted-foreground text-lg">...</p> entirely or
replace its contents with the intended descriptive text; locate this in the
component rendered in src/app/demo/milestone-progress/page.tsx where the
paragraph with className "text-muted-foreground text-lg" is defined.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/app/demo/milestone-progress/page.tsx (3)
360-371:⚠️ Potential issue | 🟡 Minor
<label>elements withouthtmlForcreate incorrect ARIA semantics.These labels are decorative text, not form labels. Replace with
<p>or<span>.Proposed fix
- <label className="text-sm font-medium">Aprobados</label> + <p className="text-sm font-medium">Aprobados</p>- <label className="text-sm font-medium">Liberados</label> + <p className="text-sm font-medium">Liberados</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/demo/milestone-progress/page.tsx` around lines 360 - 371, The <label> elements used as decorative headings around EscrowMilestoneProgressBar (e.g., the "Aprobados" and "Liberados" labels near EscrowMilestoneProgressBar usage) are not bound to form controls and create wrong ARIA semantics; replace those <label> tags with non-form semantic elements such as <p> or <span> (or a heading tag like <h4> if appropriate) so the text remains accessible without implying form association, updating the surrounding markup where EscrowMilestoneProgressBar is rendered to use the new element.
119-123:⚠️ Potential issue | 🔴 CriticalRemove the offensive comment and the empty
<p>element.Line 121 contains
{/* Please eat shit */}— this must not be merged. After removal the<p>will be empty; either delete it or add the intended description text.🐛 Proposed fix
- <p className="text-muted-foreground text-lg"> - {/* Please eat shit */} - </p> + <p className="text-muted-foreground text-lg"> + Visualización interactiva de hitos de escrow + </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/demo/milestone-progress/page.tsx` around lines 119 - 123, Remove the offensive inline comment and the empty paragraph element inside the demo page: locate the JSX block that renders the header "🎯 EscrowMilestoneProgress Demo" (around the <h1> with that text) and delete the `{/* Please eat shit */}` comment; then either remove the entire <p className="text-muted-foreground text-lg">...</p> tag if no description is needed, or replace its contents with the intended descriptive text for the demo. Ensure no leftover empty JSX comment or empty element remains.
270-270:⚠️ Potential issue | 🟡 Minor
{false}in JSX renders nothing — the prop syntax won't appear in the UI.
<CardDescription>Opción showText={false}</CardDescription>will render as "Opción showText=" because React drops the booleanfalse. Use a template literal instead.🐛 Proposed fix (also apply to line 330)
- <CardDescription>Opción showText={false}</CardDescription> + <CardDescription>{`Opción showText={false}`}</CardDescription>- <CardDescription>showText={false}</CardDescription> + <CardDescription>{`showText={false}`}</CardDescription>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/demo/milestone-progress/page.tsx` at line 270, The CardDescription child currently contains raw JSX braces that get dropped (e.g., the text "Opción showText={false}" is rendered incorrectly); update the CardDescription children so the entire snippet is passed as a string expression (for example using a template string or quoted string expression) so the braces and "false" render literally; modify the occurrence inside the CardDescription component and apply the same change to the duplicate instance referenced at the later occurrence (the other CardDescription usage around the duplicated line).
🧹 Nitpick comments (1)
src/app/demo/milestone-progress/page.tsx (1)
105-109:{} as Escrowsilently lies to the type system — every required field isundefinedat runtime.If
useEscrowMilestoneProgressor the bar/donut components ever access a field beyondtype/milestones(e.g.escrow.id), this will blow up without a compile-time warning. Consider using a more complete mock or a helper that satisfies the fullEscrowshape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/demo/milestone-progress/page.tsx` around lines 105 - 109, The mockEscrow object is asserting an empty object as Escrow which hides missing fields and can cause runtime errors when useEscrowMilestoneProgress or downstream components access other properties; replace the {} as Escrow pattern by constructing a fully populated mock that satisfies the Escrow shape (or use a test helper/factory) and copy required defaults from the Escrow interface, e.g., include id, status, parties, createdAt, etc., while still overriding type and milestones from scenario, or create a buildMockEscrow(scenario) helper used where mockEscrow is created so all required fields are present and type-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/demo/milestone-progress/page.tsx`:
- Around line 126-152: The UI strings in this component are all in Spanish;
update them to English (or replace with i18n keys) to match project convention.
Specifically change CardTitle, CardDescription and the button content generated
in the map over SCENARIOS (referencing selectedScenario and setSelectedScenario)
from Spanish to English, and ensure any other user-facing strings in this file
(e.g., checklist items elsewhere in page.tsx) are likewise converted or switched
to localized keys so text is consistent across the demo page.
- Around line 20-34: The createMultiReleaseMilestone factory currently ignores
its released and approved parameters causing milestone.flags to be empty; update
createMultiReleaseMilestone (the function returning MultiReleaseMilestone) to
populate the flags object with the passed booleans (e.g., flags: { released,
approved }) so milestone.flags?.released and milestone.flags?.approved reflect
the inputs and progress calculations work correctly.
---
Duplicate comments:
In `@src/app/demo/milestone-progress/page.tsx`:
- Around line 360-371: The <label> elements used as decorative headings around
EscrowMilestoneProgressBar (e.g., the "Aprobados" and "Liberados" labels near
EscrowMilestoneProgressBar usage) are not bound to form controls and create
wrong ARIA semantics; replace those <label> tags with non-form semantic elements
such as <p> or <span> (or a heading tag like <h4> if appropriate) so the text
remains accessible without implying form association, updating the surrounding
markup where EscrowMilestoneProgressBar is rendered to use the new element.
- Around line 119-123: Remove the offensive inline comment and the empty
paragraph element inside the demo page: locate the JSX block that renders the
header "🎯 EscrowMilestoneProgress Demo" (around the <h1> with that text) and
delete the `{/* Please eat shit */}` comment; then either remove the entire <p
className="text-muted-foreground text-lg">...</p> tag if no description is
needed, or replace its contents with the intended descriptive text for the demo.
Ensure no leftover empty JSX comment or empty element remains.
- Line 270: The CardDescription child currently contains raw JSX braces that get
dropped (e.g., the text "Opción showText={false}" is rendered incorrectly);
update the CardDescription children so the entire snippet is passed as a string
expression (for example using a template string or quoted string expression) so
the braces and "false" render literally; modify the occurrence inside the
CardDescription component and apply the same change to the duplicate instance
referenced at the later occurrence (the other CardDescription usage around the
duplicated line).
---
Nitpick comments:
In `@src/app/demo/milestone-progress/page.tsx`:
- Around line 105-109: The mockEscrow object is asserting an empty object as
Escrow which hides missing fields and can cause runtime errors when
useEscrowMilestoneProgress or downstream components access other properties;
replace the {} as Escrow pattern by constructing a fully populated mock that
satisfies the Escrow shape (or use a test helper/factory) and copy required
defaults from the Escrow interface, e.g., include id, status, parties,
createdAt, etc., while still overriding type and milestones from scenario, or
create a buildMockEscrow(scenario) helper used where mockEscrow is created so
all required fields are present and type-safe.
Comment on lines
+20
to
+34
| const createMultiReleaseMilestone = ( | ||
| description: string, | ||
| released: boolean = false, | ||
| approved: boolean = false | ||
| ): MultiReleaseMilestone => ({ | ||
| description, | ||
| receiver: "GBPZXL6K67XRQXZFGX6JYDW73OR4W2GXDW73YSDGKTX7TWWM3AZWVGW", | ||
| amount: 100, | ||
| status: "pending", | ||
| flags: { | ||
|
|
||
|
|
||
|
|
||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the MultiReleaseMilestone type definition
find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.d.ts" \) | xargs grep -l "MultiReleaseMilestone" | head -20Repository: Trustless-Work/clonable-backoffice
Length of output: 1571
🏁 Script executed:
# Read the actual file to confirm the issue at the specified lines
cat -n src/app/demo/milestone-progress/page.tsx | sed -n '15,40p'Repository: Trustless-Work/clonable-backoffice
Length of output: 776
🏁 Script executed:
# Check the `@trustless-work/escrow` package types
find . -path "*/node_modules/@trustless-work/escrow*" -name "*.d.ts" -exec head -500 {} \; 2>/dev/null | head -300Repository: Trustless-Work/clonable-backoffice
Length of output: 60
🏁 Script executed:
# Search for MultiReleaseMilestone type definition in the codebase
rg -n "MultiReleaseMilestone" src/ --type=ts --type=tsx -B2 -A10 | head -100Repository: Trustless-Work/clonable-backoffice
Length of output: 105
🏁 Script executed:
# Check the imports at the top of the file to understand where MultiReleaseMilestone comes from
head -20 src/app/demo/milestone-progress/page.tsxRepository: Trustless-Work/clonable-backoffice
Length of output: 833
🏁 Script executed:
# Search for how the flags object is used in milestone-related code
rg -n "\.flags\." src/ --type=ts --type=tsx -B2 -A2 | head -80Repository: Trustless-Work/clonable-backoffice
Length of output: 105
🏁 Script executed:
# Search for MultiReleaseMilestone type definition without type specifiers
rg -n "MultiReleaseMilestone" src/ -B2 -A10 | head -120Repository: Trustless-Work/clonable-backoffice
Length of output: 13240
🏁 Script executed:
# Check the `@trustless-work/escrow` package types - look for node_modules
find node_modules/@trustless-work/escrow -name "*.d.ts" -o -name "index.ts" 2>/dev/null | head -5Repository: Trustless-Work/clonable-backoffice
Length of output: 60
🏁 Script executed:
# Search for how flags are used in milestone-related code
rg -n "\.flags" src/ -B2 -A2 | head -100Repository: Trustless-Work/clonable-backoffice
Length of output: 10116
🏁 Script executed:
# Look for how useEscrowMilestoneProgress uses the milestone data
rg -n "useEscrowMilestoneProgress" src/ -A30 | head -80Repository: Trustless-Work/clonable-backoffice
Length of output: 8483
🏁 Script executed:
# Check the calculateMilestoneProgress function to see exactly what flags it checks for
cat -n src/components/tw-blocks/escrows/indicators/milestone-progress/useEscrowMilestoneProgress.ts | head -130Repository: Trustless-Work/clonable-backoffice
Length of output: 4832
🏁 Script executed:
# Look at the actual implementation of how flags are checked for milestones
sed -n '1,130p' src/components/tw-blocks/escrows/indicators/milestone-progress/useEscrowMilestoneProgress.tsRepository: Trustless-Work/clonable-backoffice
Length of output: 3922
🏁 Script executed:
# Check all call sites of createMultiReleaseMilestone to see what parameters are passed
rg -n "createMultiReleaseMilestone" src/app/demo/milestone-progress/page.tsx -A2 -B2Repository: Trustless-Work/clonable-backoffice
Length of output: 1319
released and approved parameters are never assigned into the returned flags object — all multi-release milestone progress calculations will show 0%.
The flags object on lines 29–33 is empty, so regardless of the booleans passed at the call sites (e.g. createMultiReleaseMilestone("Milestone 1", true, true)), every MultiReleaseMilestone will have flags.released and flags.approved as undefined. The useEscrowMilestoneProgress hook checks milestone.flags?.released === true and milestone.flags?.approved === true to count completed milestones, which will never match, making every multi-release demo scenario display 0% progress regardless of parameters.
🐛 Proposed fix — populate the flags object
const createMultiReleaseMilestone = (
description: string,
released: boolean = false,
approved: boolean = false
): MultiReleaseMilestone => ({
description,
receiver: "GBPZXL6K67XRQXZFGX6JYDW73OR4W2GXDW73YSDGKTX7TWWM3AZWVGW",
amount: 100,
status: "pending",
flags: {
-
-
-
+ released,
+ approved,
},
});📝 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
| const createMultiReleaseMilestone = ( | |
| description: string, | |
| released: boolean = false, | |
| approved: boolean = false | |
| ): MultiReleaseMilestone => ({ | |
| description, | |
| receiver: "GBPZXL6K67XRQXZFGX6JYDW73OR4W2GXDW73YSDGKTX7TWWM3AZWVGW", | |
| amount: 100, | |
| status: "pending", | |
| flags: { | |
| }, | |
| }); | |
| const createMultiReleaseMilestone = ( | |
| description: string, | |
| released: boolean = false, | |
| approved: boolean = false | |
| ): MultiReleaseMilestone => ({ | |
| description, | |
| receiver: "GBPZXL6K67XRQXZFGX6JYDW73OR4W2GXDW73YSDGKTX7TWWM3AZWVGW", | |
| amount: 100, | |
| status: "pending", | |
| flags: { | |
| released, | |
| approved, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/demo/milestone-progress/page.tsx` around lines 20 - 34, The
createMultiReleaseMilestone factory currently ignores its released and approved
parameters causing milestone.flags to be empty; update
createMultiReleaseMilestone (the function returning MultiReleaseMilestone) to
populate the flags object with the passed booleans (e.g., flags: { released,
approved }) so milestone.flags?.released and milestone.flags?.approved reflect
the inputs and progress calculations work correctly.
Comment on lines
+126
to
+152
| <Card> | ||
| <CardHeader> | ||
| <CardTitle>Selecciona un Escenario</CardTitle> | ||
| <CardDescription> | ||
| Prueba diferentes configuraciones de escrows y hitos | ||
| </CardDescription> | ||
| </CardHeader> | ||
| <CardContent className="space-y-4"> | ||
| <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-3"> | ||
| {(Object.entries(SCENARIOS) as [ScenarioKey, typeof SCENARIOS[ScenarioKey]][]).map( | ||
| ([key, { description }]) => ( | ||
| <Button | ||
| key={key} | ||
| variant={selectedScenario === key ? "default" : "outline"} | ||
| className="h-auto p-3 text-left justify-start" | ||
| onClick={() => setSelectedScenario(key)} | ||
| > | ||
| <div> | ||
| <div className="font-medium">{key}</div> | ||
| <div className="text-xs text-muted-foreground">{description}</div> | ||
| </div> | ||
| </Button> | ||
| ) | ||
| )} | ||
| </div> | ||
| </CardContent> | ||
| </Card> |
There was a problem hiding this comment.
All user-facing text is in Spanish — consider i18n or switching to English per maintainer feedback.
Reviewer JoelVR17 previously requested removing Spanish comments. The entire demo page (card titles, descriptions, button labels, checklist items) is in Spanish. If the project convention is English, this should be updated throughout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/demo/milestone-progress/page.tsx` around lines 126 - 152, The UI
strings in this component are all in Spanish; update them to English (or replace
with i18n keys) to match project convention. Specifically change CardTitle,
CardDescription and the button content generated in the map over SCENARIOS
(referencing selectedScenario and setSelectedScenario) from Spanish to English,
and ensure any other user-facing strings in this file (e.g., checklist items
elsewhere in page.tsx) are likewise converted or switched to localized keys so
text is consistent across the demo page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature: Escrow Milestone Progress Components
Overview
This PR introduces new UI components for visualizing escrow milestone progress, along with supporting types, demo implementations, and documentation.
Changes Included
Dependencies
Milestone Progress Components
Documentation
Purpose
These components provide a reusable and dynamic way to represent escrow milestone progress, improving clarity and user experience in escrow-related views.
Impact
Closes #17
Summary by CodeRabbit
New Features
Documentation