fix(flows): frontend residual minors — stale route, edge id collisions, cron clamp, unvalidated status - #5292
Conversation
📝 WalkthroughWalkthroughThe PR adds safe fallbacks for unknown flow-run statuses, stricter cron validation, stable React Flow connection IDs, and ChangesFlow-run status fallbacks
Cron validation
Stable connection IDs
Run-page navigation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
80a8f65 to
2e94aaa
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e94aaa04f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Every N hours: `M */N * * dow` | ||
| const hourStep = parseStep(hour); | ||
| if (hourStep && /^\d+$/.test(min)) { | ||
| if (hourStep && /^\d+$/.test(min) && hourStep.step >= 1 && hourStep.step <= 23) { |
There was a problem hiding this comment.
In the hourly cron path, a custom expression like 90 */2 * * * still parses as a visual hours spec with minute: 90; the next unrelated ScheduleField edit then recompiles through buildCron() and silently rewrites it to 59 */2 * * *. This leaves the same data-loss behavior this patch is trying to avoid for step values, so this branch should also require the minute field to be within 0..59 before returning a CronSpec.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — thanks for catching this. The same data-loss hole existed in three more places than the hourly minute field you flagged:
- Hourly
minute(the one you named):90 */2 * * *now returnsnullinstead of parsing tominute: 90and getting silently rewritten to59 */2 * * *on the next unrelated edit. - Daily
minuteandhour: same shape —buildCronclamps both (0-59 / 0-23) butparseCronwas not validating either before this fix.75 9 * * *and30 25 * * *now both returnnull. - Weekday list:
normalizeWeekdayssilently drops any day outside 0-6 (after mapping 7→Sun), so a mixed list like1,8used to parse to just[1]— same "narrow and move on" bug for thedowfield.parseCronnow rejects the whole field (→null) if any entry is outside cron's valid 0-7 range.
Added a shared parseBoundedInt(field, max) helper used by both the hourly and daily branches so the range check lives in one place.
Verified the caller degrades correctly: ScheduleField seeds advanced from parsed === null, so a rejected expression still renders in the raw/editable cron text field with a "custom schedule" summary from describeCron — confirmed by reading ScheduleField.tsx (no test needed there since the wiring was already covered by F-m7's existing test).
Tests added in cron.test.ts (app/src/lib/flows/cron.ts:131, app/src/lib/flows/cron.test.ts):
- out-of-range hourly minute (
90 */2 * * *,60 */2 * * *) - out-of-range daily minute/hour, individually and together
- out-of-range weekday, both standalone (
8) and mixed into a valid list (1,8) - boundary values (0/23/59/7) still accepted
- a battery of in-range hourly/daily/weekly crons proven unaffected (round-trip unchanged)
Verified real counts: tsc --noEmit clean, vitest run on src/lib/flows src/components/flows src/pages — 81 files / 867 tests passed (cron.test.ts alone: 35 tests), prettier --check . clean. Amended into the existing top commit and pushed.
describeNode/describeCron/describeEveryMs/describeSchedule returned hardcoded English literals rendered on every canvas node card and in the schedule field, violating the project's useT() rule. Thread `t` (and `locale`, for weekday names) through these pure modules mirroring the existing runStepSummary.ts pattern, add flows.nodeSummary.*/ flows.cron.* keys to en.ts plus real translations across all 14 locales, and derive weekday names from Intl.DateTimeFormat against the active locale instead of hand-translated WEEKDAY_SHORT/WEEKDAY_INITIAL arrays. Allowlist two pure-placeholder strings (HTTP method+URL, quoted prompt+model) in the i18n-find-english leftover-English checker, same as existing entries like channels.activeRouteValue.
2e94aaa to
f7485fa
Compare
There was a problem hiding this comment.
graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…s, cron clamp, unvalidated status (F-m1/F-m6/F-m7/F-m8) - F-m1: WorkflowsRun's cold-deep-link back-fallback pointed at /intelligence?tab=workflows, which redirects to /settings/notifications. Point it at the real workflows route (/flows). - F-m6: EditableFlowCanvas's onConnect used React Flow's default concatenated edge id, the exact collision-prone scheme graphAdapter's edgeId() exists to avoid. Add connectionEdgeId() and use it so editor-created edges match adapter-created ones. - F-m7: parseCron accepted any numeric cron step (e.g. */90), which the visual editor's next unrelated patch() would silently recompile through buildCron's clamp, rewriting the stored expression to */59. Reject out-of-range steps so they round-trip untouched through the advanced cron field instead. - F-m8: FlowRunStatus/FlowRunsDrawer/FlowRunsSidebar/FlowRunInspectorDrawer indexed status maps with an unvalidated wire status, rendering "undefined" for an unrecognized FlowRunStatus. Add flowRunStatusLabel/AccentClass/DotClass with the same fallback pattern WorkflowRunsPage.tsx already used, applied consistently everywhere the status maps are read. F-m4 (file-size rule) evaluated and deliberately skipped — see PR/task notes.
f7485fa to
3c95c94
Compare
There was a problem hiding this comment.
graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…minors # Conflicts: # app/src/lib/i18n/es.ts # app/src/lib/i18n/pt.ts
There was a problem hiding this comment.
graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![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 (2)
app/src/components/flows/FlowRunStatus.test.tsx (1)
124-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the mapped translation key in the recognized-status test.
The mock
treturns the fallback for every key. Therefore, Line 128 passes even ifflowRunStatusLabelskipsFLOW_RUN_STATUS_KEYor uses the wrong key. Use a spy and assert thattreceivesflowRuns.status.completed, then assert the localized result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/flows/FlowRunStatus.test.tsx` around lines 124 - 129, Update the recognized-status case in flowRunStatusLabel so the translation mock spies on its invocation and verifies it receives the key flowRuns.status.completed with the expected fallback, then assert the returned localized value. Keep the unknown-status fallback assertion unchanged.app/src/pages/WorkflowsRun.test.tsx (1)
12-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCreate the shared mock with
vi.hoisted.Vitest hoists
vi.mockbefore imports and documentsvi.hoistedfor values referenced by mock factories. The current factory capturesnavigateMockin a returned function, so it does not show a confirmed immediate TDZ failure. Move the mock creation tovi.hoistedto avoid relying on deferred evaluation. (vitest.dev)Suggested fix
-const navigateMock = vi.fn(); +const navigateMock = vi.hoisted(() => vi.fn());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/WorkflowsRun.test.tsx` around lines 12 - 17, Move the navigateMock creation in WorkflowsRun.test.tsx into a vi.hoisted callback, then have the react-router-dom vi.mock factory reference that hoisted mock while preserving the existing useNavigate behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/components/flows/FlowRunStatus.tsx`:
- Around line 48-54: Update flowRunStatusAccentClass, flowRunStatusDotClass, and
the corresponding third status-map lookup to verify each status is an own key
before returning its mapped value, otherwise use the neutral fallback. Add
regression tests covering toString, constructor, and __proto__ for all three
lookup paths.
In `@app/src/lib/flows/cron.test.ts`:
- Around line 212-214: Update the test for weekdayShortLabel to assert the exact
English fallback label for the unsupported locale, not only that the call does
not throw. Keep the existing unsupported locale input and weekday index, and
verify the returned value matches the expected English weekday abbreviation.
In `@app/src/lib/flows/graphAdapter.test.ts`:
- Around line 434-448: The connectionEdgeId normalization test currently covers
only explicit null handles and omits endpoint cases. Update the test around
connectionEdgeId to add a case with sourceHandle and targetHandle omitted, plus
cases with null source or target endpoints, and assert each normalizes
consistently with the corresponding explicit main/default representation.
In `@app/src/pages/WorkflowsRun.test.tsx`:
- Around line 43-67: Update the two back-navigation tests around the cold-link
and in-app history cases to explicitly set window.history.state via
History.replaceState rather than relying on ambient state or redefining the
property. Save the prior history state before the history-entry test and restore
it in its finally block, while ensuring the cold-link test uses an explicit
no-entry state and both tests retain their existing navigation assertions.
In `@app/src/pages/WorkflowsRun.tsx`:
- Line 41: Update the fallback navigation in WorkflowsRun to call
navigate('/flows', { replace: true }) when window.history.state?.idx is missing
or zero, while preserving navigate(-1) for positive indices. Update WorkflowsRun
tests to assert the replace options object for the fallback.
---
Nitpick comments:
In `@app/src/components/flows/FlowRunStatus.test.tsx`:
- Around line 124-129: Update the recognized-status case in flowRunStatusLabel
so the translation mock spies on its invocation and verifies it receives the key
flowRuns.status.completed with the expected fallback, then assert the returned
localized value. Keep the unknown-status fallback assertion unchanged.
In `@app/src/pages/WorkflowsRun.test.tsx`:
- Around line 12-17: Move the navigateMock creation in WorkflowsRun.test.tsx
into a vi.hoisted callback, then have the react-router-dom vi.mock factory
reference that hoisted mock while preserving the existing useNavigate behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8826104-d2d1-4eea-926f-5449efafa9ad
📒 Files selected for processing (16)
app/src/components/flows/FlowRunInspectorDrawer.tsxapp/src/components/flows/FlowRunStatus.test.tsxapp/src/components/flows/FlowRunStatus.tsxapp/src/components/flows/FlowRunsDrawer.test.tsxapp/src/components/flows/FlowRunsDrawer.tsxapp/src/components/flows/FlowRunsSidebar.test.tsxapp/src/components/flows/FlowRunsSidebar.tsxapp/src/components/flows/__tests__/FlowRunInspectorDrawer.test.tsxapp/src/components/flows/canvas/EditableFlowCanvas.tsxapp/src/components/flows/canvas/nodeConfig/__tests__/ScheduleField.test.tsxapp/src/lib/flows/cron.test.tsapp/src/lib/flows/cron.tsapp/src/lib/flows/graphAdapter.test.tsapp/src/lib/flows/graphAdapter.tsapp/src/pages/WorkflowsRun.test.tsxapp/src/pages/WorkflowsRun.tsx
| export function flowRunStatusAccentClass(status: FlowRunStatusValue): string { | ||
| return FLOW_RUN_STATUS_ACCENT[status] ?? UNKNOWN_STATUS_ACCENT; | ||
| } | ||
|
|
||
| /** Runtime-safe dot class lookup — falls back for an unrecognized status. */ | ||
| export function flowRunStatusDotClass(status: FlowRunStatusValue): string { | ||
| return FLOW_RUN_STATUS_DOT[status] ?? UNKNOWN_STATUS_DOT; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 'FLOW_RUN_STATUS_(ACCENT|DOT|KEY)' app/src/components/flows/FlowRunStatus.tsxRepository: tinyhumansai/openhuman
Length of output: 3768
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- FlowRunStatus.tsx ---'
cat -n app/src/components/flows/FlowRunStatus.tsx | sed -n '1,130p'
printf '%s\n' '--- FlowRunStatus and status type references ---'
rg -n -C 5 'type FlowRunStatus|interface FlowRunStatus|FlowRunStatusValue|flowRunStatus(Label|AccentClass|DotClass)' app/src
printf '%s\n' '--- prototype-key behavior of the current lookup shape ---'
node - <<'JS'
const accent = {
running: 'running-class',
completed: 'completed-class',
};
const dot = {
running: 'running-dot',
completed: 'completed-dot',
};
const key = {
running: 'flowRuns.status.running',
completed: 'flowRuns.status.completed',
};
for (const status of ['toString', 'constructor', '__proto__', 'missing']) {
const accentValue = accent[status] ?? 'unknown-accent';
const dotValue = dot[status] ?? 'unknown-dot';
const labelKey = key[status];
const label = labelKey ? labelKey : status.replace(/_/g, ' ');
console.log(JSON.stringify({
status,
accentType: typeof accentValue,
accentValue: String(accentValue),
dotType: typeof dotValue,
dotValue: String(dotValue),
labelKeyType: typeof labelKey,
label,
}));
}
JSRepository: tinyhumansai/openhuman
Length of output: 31144
Guard status-map lookups against inherited keys.
Statuses such as toString, constructor, and __proto__ return inherited values from all three maps instead of the neutral fallback. Use own-property checks or null-prototype maps, and add regression tests for these statuses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/components/flows/FlowRunStatus.tsx` around lines 48 - 54, Update
flowRunStatusAccentClass, flowRunStatusDotClass, and the corresponding third
status-map lookup to verify each status is an own key before returning its
mapped value, otherwise use the neutral fallback. Add regression tests covering
toString, constructor, and __proto__ for all three lookup paths.
| it('falls back to English for an unsupported locale tag rather than throwing', () => { | ||
| expect(() => weekdayShortLabel(0, 'not-a-real-locale')).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the fallback result.
This test only checks that weekdayShortLabel does not throw. It passes if the function returns a non-English default locale instead of the required English fallback. Assert the returned label.
Proposed fix
- expect(() => weekdayShortLabel(0, 'not-a-real-locale')).not.toThrow();
+ expect(weekdayShortLabel(0, 'not-a-real-locale')).toBe('Sun');📝 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.
| it('falls back to English for an unsupported locale tag rather than throwing', () => { | |
| expect(() => weekdayShortLabel(0, 'not-a-real-locale')).not.toThrow(); | |
| }); | |
| it('falls back to English for an unsupported locale tag rather than throwing', () => { | |
| expect(weekdayShortLabel(0, 'not-a-real-locale')).toBe('Sun'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/lib/flows/cron.test.ts` around lines 212 - 214, Update the test for
weekdayShortLabel to assert the exact English fallback label for the unsupported
locale, not only that the call does not throw. Keep the existing unsupported
locale input and weekday index, and verify the returned value matches the
expected English weekday abbreviation.
| it('defaults null/undefined handles to the "main" port, matching isValidFlowConnection', () => { | ||
| const withNullHandles = connectionEdgeId({ | ||
| source: 'a', | ||
| sourceHandle: null, | ||
| target: 'b', | ||
| targetHandle: null, | ||
| }); | ||
| const withExplicitMain = connectionEdgeId({ | ||
| source: 'a', | ||
| sourceHandle: 'main', | ||
| target: 'b', | ||
| targetHandle: 'main', | ||
| }); | ||
| expect(withNullHandles).toBe(withExplicitMain); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test omitted handles and endpoints.
Line 434 states that the test covers undefined handles, but both handles are null. The suite also does not test the new missing-endpoint normalization. Add cases with omitted handle fields and null endpoints.
Proposed test additions
const withExplicitMain = connectionEdgeId({
source: 'a',
sourceHandle: 'main',
target: 'b',
targetHandle: 'main',
});
expect(withNullHandles).toBe(withExplicitMain);
+
+ const withOmittedHandles = connectionEdgeId({ source: 'a', target: 'b' });
+ expect(withOmittedHandles).toBe(withExplicitMain);
+
+ expect(
+ connectionEdgeId({
+ source: null,
+ sourceHandle: null,
+ target: null,
+ targetHandle: null,
+ })
+ ).toBe(edgeId({ from_node: '', from_port: 'main', to_node: '', to_port: 'main' }));As per coding guidelines, “Untested code is incomplete; add tests for new or changed behavior.”
📝 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.
| it('defaults null/undefined handles to the "main" port, matching isValidFlowConnection', () => { | |
| const withNullHandles = connectionEdgeId({ | |
| source: 'a', | |
| sourceHandle: null, | |
| target: 'b', | |
| targetHandle: null, | |
| }); | |
| const withExplicitMain = connectionEdgeId({ | |
| source: 'a', | |
| sourceHandle: 'main', | |
| target: 'b', | |
| targetHandle: 'main', | |
| }); | |
| expect(withNullHandles).toBe(withExplicitMain); | |
| }); | |
| it('defaults null/undefined handles to the "main" port, matching isValidFlowConnection', () => { | |
| const withNullHandles = connectionEdgeId({ | |
| source: 'a', | |
| sourceHandle: null, | |
| target: 'b', | |
| targetHandle: null, | |
| }); | |
| const withExplicitMain = connectionEdgeId({ | |
| source: 'a', | |
| sourceHandle: 'main', | |
| target: 'b', | |
| targetHandle: 'main', | |
| }); | |
| expect(withNullHandles).toBe(withExplicitMain); | |
| const withOmittedHandles = connectionEdgeId({ source: 'a', target: 'b' }); | |
| expect(withOmittedHandles).toBe(withExplicitMain); | |
| expect( | |
| connectionEdgeId({ | |
| source: null, | |
| sourceHandle: null, | |
| target: null, | |
| targetHandle: null, | |
| }) | |
| ).toBe(edgeId({ from_node: '', from_port: 'main', to_node: '', to_port: 'main' })); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/lib/flows/graphAdapter.test.ts` around lines 434 - 448, The
connectionEdgeId normalization test currently covers only explicit null handles
and omits endpoint cases. Update the test around connectionEdgeId to add a case
with sourceHandle and targetHandle omitted, plus cases with null source or
target endpoints, and assert each normalizes consistently with the corresponding
explicit main/default representation.
Source: Coding guidelines
| it('falls back to /flows (not the dead /intelligence?tab=workflows route) on a cold deep-link with no history', () => { | ||
| // No `window.history.state.idx` — matches a fresh deep-link with no | ||
| // in-app history entry to go back to (F-m1: /intelligence redirects to | ||
| // /settings/notifications, so the runner must not target it). | ||
| navigateMock.mockClear(); | ||
| render_(); | ||
| fireEvent.click(screen.getByRole('button', { name: 'common.back' })); | ||
| expect(navigateMock).toHaveBeenCalledWith('/flows'); | ||
| expect(navigateMock).not.toHaveBeenCalledWith(expect.stringContaining('/intelligence')); | ||
| }); | ||
|
|
||
| it('goes back in history instead when an in-app history entry exists', () => { | ||
| navigateMock.mockClear(); | ||
| const originalDescriptor = Object.getOwnPropertyDescriptor(window.history, 'state'); | ||
| Object.defineProperty(window.history, 'state', { configurable: true, value: { idx: 1 } }); | ||
| try { | ||
| render_(); | ||
| fireEvent.click(screen.getByRole('button', { name: 'common.back' })); | ||
| expect(navigateMock).toHaveBeenCalledWith(-1); | ||
| } finally { | ||
| if (originalDescriptor) { | ||
| Object.defineProperty(window.history, 'state', originalDescriptor); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="app/src/pages/WorkflowsRun.test.tsx"
printf '%s\n' "== file outline =="
ast-grep outline "$file" --lang tsx
printf '%s\n' "== relevant source =="
sed -n '1,130p' "$file"
printf '%s\n' "== history.state references in the test file =="
rg -n -C 3 'history\.state|replaceState|defineProperty|navigateMock|render_' "$file"
printf '%s\n' "== neighboring test setup and history references =="
rg -n -C 3 'history\.state|replaceState|defineProperty' app/src --glob '*.{test,spec}.{ts,tsx}' | head -250Repository: tinyhumansai/openhuman
Length of output: 27939
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' "== WorkflowsRun implementation =="
ast-grep outline app/src/pages/WorkflowsRun.tsx --lang tsx
sed -n '1,180p' app/src/pages/WorkflowsRun.tsx
printf '%s\n' "== Vitest configuration and setup files =="
fd -i 'vitest|vite|setup' . -t f | head -100
printf '%s\n' "== test environment declarations =="
rg -n -C 3 'environment|jsdom|happy-dom|setupFiles|history' --glob 'vitest*.{ts,js,mjs,cjs}' --glob 'vite*.{ts,js,mjs,cjs}' --glob '*setup*.{ts,tsx,js,ts}' . | head -250
printf '%s\n' "== dependency metadata =="
rg -n -C 2 '"(jsdom|happy-dom|vitest|react-router-dom)"' package.json package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null | head -200 || true
printf '%s\n' "== ECMAScript property-restoration probe =="
node - <<'JS'
const prototype = {};
Object.defineProperty(prototype, 'state', {
configurable: true,
get() { return this._state; },
});
const history = Object.create(prototype);
const originalDescriptor = Object.getOwnPropertyDescriptor(history, 'state');
Object.defineProperty(history, 'state', { configurable: true, value: { idx: 1 } });
if (originalDescriptor) {
Object.defineProperty(history, 'state', originalDescriptor);
}
console.log(JSON.stringify({
originalDescriptorWasUndefined: originalDescriptor === undefined,
ownDescriptorRemains: Object.prototype.hasOwnProperty.call(history, 'state'),
state: history.state,
}));
JSRepository: tinyhumansai/openhuman
Length of output: 8528
Set and restore window.history.state explicitly.
The cold-link test depends on ambient history state. The history-entry test can leave an own state property when no own descriptor existed. Use History.replaceState to set explicit states and restore the previous state in finally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/pages/WorkflowsRun.test.tsx` around lines 43 - 67, Update the two
back-navigation tests around the cold-link and in-app history cases to
explicitly set window.history.state via History.replaceState rather than relying
on ambient state or redefining the property. Save the prior history state before
the history-entry test and restore it in its finally block, while ensuring the
cold-link test uses an explicit no-entry state and both tests retain their
existing navigation assertions.
| (window.history.state?.idx ?? 0) > 0 | ||
| ? navigate(-1) | ||
| : navigate('/intelligence?tab=workflows') | ||
| (window.history.state?.idx ?? 0) > 0 ? navigate(-1) : navigate('/flows') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file outline ---'
ast-grep outline app/src/pages/WorkflowsRun.tsx --view expanded
printf '%s\n' '--- target implementation ---'
cat -n app/src/pages/WorkflowsRun.tsx | sed -n '1,100p'
printf '%s\n' '--- related tests ---'
fd -i 'WorkflowsRun' app/src | xargs -r -n1 sh -c 'echo "--- $0"; cat -n "$0" | sed -n "1,240p"'
printf '%s\n' '--- route and navigation references ---'
rg -n --glob '*.{ts,tsx}' "WorkflowsRun|/flows|navigate\\(-1\\)|history\\.state|replace: true" app/srcRepository: tinyhumansai/openhuman
Length of output: 47426
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- router dependency versions ---'
python3 - <<'PY'
import json
from pathlib import Path
for name in ('package.json', 'app/package.json'):
p = Path(name)
if p.exists():
data = json.loads(p.read_text())
deps = {**data.get('dependencies', {}), **data.get('devDependencies', {})}
print(name, 'react-router-dom=', deps.get('react-router-dom'))
PY
printf '%s\n' '--- navigation guard implementation ---'
cat -n app/src/components/settings/hooks/useSettingsNavigation.ts | sed -n '120,190p'
printf '%s\n' '--- deterministic history model ---'
python3 - <<'PY'
def navigate(history, index, destination, replace=False):
if replace:
history[index] = destination
else:
history = history[:index + 1] + [destination] + history[index + 1:]
index += 1
return history, index
initial = ['external-referrer', '/flows/run?workflow=w1']
push_history, push_index = navigate(initial[:], 1, '/flows')
replace_history, replace_index = navigate(initial[:], 1, '/flows', replace=True)
print('push:', push_history, 'index=', push_index, 'back=', push_history[push_index - 1])
print('replace:', replace_history, 'index=', replace_index, 'back=', replace_history[replace_index - 1])
assert push_history[push_index - 1] == '/flows/run?workflow=w1'
assert replace_history[replace_index - 1] == 'external-referrer'
PY
printf '%s\n' '--- current fallback and test assertions ---'
rg -n -C 3 "navigate\\('/flows'\\)|toHaveBeenCalledWith\\('/flows'\\)" app/src/pages/WorkflowsRun.tsx app/src/pages/WorkflowsRun.test.tsxRepository: tinyhumansai/openhuman
Length of output: 4825
Replace the cold-link entry instead of pushing a new entry.
When idx is missing or 0, call navigate('/flows', { replace: true }) to prevent Back from returning to the runner. Update app/src/pages/WorkflowsRun.test.tsx to assert the options object.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/pages/WorkflowsRun.tsx` at line 41, Update the fallback navigation in
WorkflowsRun to call navigate('/flows', { replace: true }) when
window.history.state?.idx is missing or zero, while preserving navigate(-1) for
positive indices. Update WorkflowsRun tests to assert the replace options object
for the fallback.
1 participant
Summary
edgeId()was written to avoid.undefinedin three more places.Problem
F-m1 —
WorkflowsRun.tsx's back-fallback navigated to/intelligence?tab=workflows, butAppRoutes.tsxredirects/intelligenceto/settings/notifications. A cold deep-link to/workflows/runfollowed by Back landed the user in notification settings.F-m6 —
EditableFlowCanvas.tsxcreated edges withaddEdge(connection, current), taking React Flow's default concatenated id — precisely the schemegraphAdapter.ts'sedgeId()exists to avoid. That helper JSON-encodes the 4-tuple because plain concatenation collides: node"a-b"+ port"c"produces the same id as node"a"+ port"b-c". Editor-created edges could therefore collide and duplicate React keys until the next load regenerated ids.F-m7 —
parseCronaccepted any numeric step (*/90 * * * *→interval: 90, described as "Every 90 minutes"), but the visual editor's nextpatch()recompiled throughbuildCron's clamp (1–59 minutes / 1–23 hours) and silently changed the stored expression to*/59— without the user touching the interval field.F-m8 —
FlowRunStatus.tsxand its consumers indexed statusRecords directly with a value cast straight off the wire and never validated, so a future or unknownFlowRunStatusyielded anundefinedclass and anundefinedi18n key.WorkflowRunsPage.tsxalready defended with a fallback; the drawer, sidebar, and inspector did not.Solution
/flowsroute.connectionEdgeId()ingraphAdapter.tsthat appliesedgeId()'s collision-free tuple id to a liveConnection, andonConnectnow passes it explicitly. Editor-created ids now match adapter-created ones.parseCronrejects a step outside the rangebuildCronclamps to, so the expression is preserved as an opaque custom cron rather than being silently rewritten later. Chosen over surfacing the clamp in the UI because it is purely local, needs no new UI or i18n surface, and never rewrites what the user actually wrote.flowRunStatusLabel/flowRunStatusAccentClass/flowRunStatusDotClassmirroring the fallback patternWorkflowRunsPage.tsxalready uses, and applies them at every direct-index site. This includesFlowRunInspectorDrawer.tsx, which had the identical bug though the review had not named it.F-m4(file sizes) was evaluated and deliberately skipped.EditableFlowCanvas.tsx(~875 lines) andnodeConfigFields.tsx(~577) exceed the ~500-line preference, but their state is tightly coupled (undo/redo history, drag/connect callbacks) and a clean extraction would have ballooned this diff and risked behaviour regressions for a line-count win. A smaller verifiable diff was judged worth more.Submission Checklist
N/A: bug fixes to existing UI, no feature rows added/removed/renamed## Related—N/A: no matrix feature rows affectedN/A: no release-cut surface affectedCloses #NNN—N/A: found by code review, no tracking issue filed yetImpact
app/src). No Rust, no Tauri.pnpm i18n:checkexit 0, 0 missing/extra.connectionEdgeIdis additive. Existing persisted edge ids are unchanged; only newly created editor edges get the corrected scheme.Related
N/AF-m4file-size extraction, deliberately deferred (see above).FlowCanvasPage.tsx/FlowsPage.tsx/flowsApi.ts).AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/flows-residual-minors3ded79653(plus fix(flows): localize node-summary and cron descriptions (i18n ×14) #5289's3a39433dfas its base)Validation Run
pnpm --filter openhuman-app format:check— lint clean on touched files (0 errors; pre-existing warnings elsewhere untouched)pnpm typecheck— passesvitest run src/lib/flows src/components/flows src/pages→ 852 passed, 80 files, 0 failedapp/src-tauriuntouchedValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
*/90cron is no longer rewritten to*/59behind the user's back.Parity Contract
WorkflowRunsPage.tsx's existing fallback rather than introducing a second pattern.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
undefined.Tests