Fix nested horizontal scroll in schedule period form#1883
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Files:
packages/transition-frontend/src/components/**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
Files:
🧠 Learnings (15)📓 Common learnings📚 Learning: 2025-12-17T22:46:52.336ZApplied to files:
📚 Learning: 2026-02-03T20:40:36.982ZApplied to files:
📚 Learning: 2026-02-03T20:40:36.982ZApplied to files:
📚 Learning: 2025-09-25T16:09:00.577ZApplied to files:
📚 Learning: 2025-11-04T19:35:10.045ZApplied to files:
📚 Learning: 2026-01-28T18:00:30.001ZApplied to files:
📚 Learning: 2026-02-06T19:06:19.438ZApplied to files:
📚 Learning: 2025-11-25T21:28:28.491ZApplied to files:
📚 Learning: 2025-11-25T21:24:26.424ZApplied to files:
📚 Learning: 2025-11-18T19:37:07.564ZApplied to files:
📚 Learning: 2026-02-03T20:40:36.982ZApplied to files:
📚 Learning: 2026-04-06T16:24:55.134ZApplied to files:
📚 Learning: 2025-09-25T16:40:55.314ZApplied to files:
📚 Learning: 2026-01-21T14:59:52.530ZApplied to files:
🔇 Additional comments (6)
WalkthroughInputSelect.tsx adds an optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx`:
- Around line 181-187: The inline layout styles in the TransitSchedulePeriod
component (the JSX element with style={{ display:'flex', flexDirection:'row',
flexShrink:0, borderRight:'1px solid rgba(255,255,255,0.2)',
alignItems:'flex-start', width:'fit-content' }}) should be moved into SCSS:
create descriptive class(es) (e.g., .transit-schedule-period-row and variants
used in the same component) in the component's SCSS module, implement the
equivalent CSS (flex, flex-direction, flex-shrink, border-right, align-items,
width handling), replace the inline style props on the corresponding JSX
elements with className references, and apply the same refactor for the other
inline style occurrences in this file (the other similar style usages flagged in
the review). Ensure class names are unique to TransitSchedulePeriod to avoid
collisions.
- Around line 81-93: The code currently checks actualOutboundPathId and
actualInboundPathId with !== undefined which lets null bypass the single-choice
fallback; change the checks to treat null as unset—either use a
nullish-coalescing expression (e.g., use schedulePeriod.outbound_path_id ??
(outboundPathsChoices.length === 1 ? outboundPathsChoices[0].value : '')) and
similarly for inbound (schedulePeriod.inbound_path_id ??
(inboundPathsChoices.length === 1 ? inboundPathsChoices[0].value : undefined)),
or replace the !== undefined checks with != null for both actualOutboundPathId
and actualInboundPathId so the single-choice fallback on outboundPathsChoices
and inboundPathsChoices triggers when values are null.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9eefd364-9ae4-42e3-bd41-991a8e4f0f97
📒 Files selected for processing (2)
packages/chaire-lib-frontend/src/components/input/InputSelect.tsxpackages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test-sequential (24.x)
- GitHub Check: code-lint
- GitHub Check: build-and-test (24.x)
- GitHub Check: pr-build-check
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
**/*.{ts,tsx}: Use UUIDs as primary keys for most entities in the database
Use the Status/Result pattern from chaire-lib-common for service functions and handlers
Indent using 4 spaces
Do not use trailing spaces, even in comments or documentation; replace lines with only spaces with blank lines
Use parentheses for arrow function parameters
Document non-trivial functions and attributes using JSDoc or inline comments when short
Use Geographic data in GeoJSON format with coordinates as [longitude, latitude]
Use i18next and thet()function for all user-facing strings to support internationalization
Files:
packages/chaire-lib-frontend/src/components/input/InputSelect.tsxpackages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx
packages/transition-frontend/src/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/project-rule.mdc)
React components should use Redux for state management, i18next for translations, and SCSS for styling
Files:
packages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/**/*.tsx : React components should use Redux for state management, i18next for translations, and SCSS for styling
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
📚 Learning: 2025-12-17T22:46:52.336Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1666
File: packages/chaire-lib-frontend/package.json:60-60
Timestamp: 2025-12-17T22:46:52.336Z
Learning: In packages/chaire-lib-frontend/package.json, the `testing-library/dom` devDependency is required for tests to pass even though it's not explicitly imported. It was previously provided as a transitive dependency through `react-select-event`, but after removing `react-select-event`, it must be explicitly declared.
Applied to files:
packages/chaire-lib-frontend/src/components/input/InputSelect.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/**/*.tsx : React components should use Redux for state management, i18next for translations, and SCSS for styling
Applied to files:
packages/chaire-lib-frontend/src/components/input/InputSelect.tsxpackages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx
📚 Learning: 2026-02-03T20:40:36.982Z
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/routers/*.tsx : Application routing should be defined in `TransitionRouter.tsx` using React Router with feature-based route organization
Applied to files:
packages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx
📚 Learning: 2025-09-25T16:09:00.577Z
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1494
File: packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts:42-45
Timestamp: 2025-09-25T16:09:00.577Z
Learning: In packages/transition-backend/src/services/transitRouting/TrAccessibilityMapBatch.ts, greenscientist deferred adding input file validation using `if (!job.hasInputFile())` and requested to be reminded about it in a later PR.
Applied to files:
packages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx
📚 Learning: 2025-11-04T19:35:10.045Z
Learnt from: tahini
Repo: chairemobilite/transition PR: 1554
File: packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts:23-36
Timestamp: 2025-11-04T19:35:10.045Z
Learning: In the OdTripSimulationMethod (packages/transition-common/src/services/simulation/simulationMethod/OdTripSimulationMethod.ts), the team plans to move away from the datasource path and use batch calculations instead. The current non-functional dataSourceId selector will be reworked as part of this architectural change.
Applied to files:
packages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx
📚 Learning: 2026-01-21T14:59:52.530Z
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1592
File: packages/transition-frontend/src/components/map/CircleSpinnerExtension.tsx:37-44
Timestamp: 2026-01-21T14:59:52.530Z
Learning: In TSX files under packages/transition-frontend/src that currently rely on deck.gl's this.context.animationProps.time for animation timing, prefer window.performance.now() (or performance.now()) to measure elapsed time. Replace time-based calculations tied to deck.gl with direct high-resolution timestamps to improve accuracy and decouple from the rendering library. Ensure animations use delta = now - lastTime, and update lastTime accordingly for smooth, frame-rate-independent timing.
Applied to files:
packages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx
🔇 Additional comments (2)
packages/chaire-lib-frontend/src/components/input/InputSelect.tsx (1)
27-27: Nice extension ofInputSelectfor per-instance sizing.The optional
styleprop is integrated cleanly and keeps backward compatibility.Also applies to: 48-50, 99-100
packages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx (1)
354-359: Good guard on schedule generation eligibility.Requiring a non-blank outbound path before showing Generate is the right UX and matches backend constraints.
| actualOutboundPathId !== undefined | ||
| ? actualOutboundPathId | ||
| : outboundPathsChoices.length === 1 | ||
| ? outboundPathsChoices[0].value | ||
| : ''; | ||
|
|
||
| const actualInboundPathId = schedulePeriod.inbound_path_id; | ||
| const inboundPathId = | ||
| actualInboundPathId || (inboundPathsChoices.length === 1 ? inboundPathsChoices[0].value : undefined); | ||
| actualInboundPathId !== undefined | ||
| ? actualInboundPathId | ||
| : inboundPathsChoices.length === 1 | ||
| ? inboundPathsChoices[0].value | ||
| : undefined; |
There was a problem hiding this comment.
Handle null as unset in path selection fallback.
Using only !== undefined lets null pass through, so single-choice fallback is skipped for null-valued periods.
Suggested fix
const outboundPathId =
- actualOutboundPathId !== undefined
+ actualOutboundPathId !== undefined && actualOutboundPathId !== null
? actualOutboundPathId
: outboundPathsChoices.length === 1
? outboundPathsChoices[0].value
: '';
const inboundPathId =
- actualInboundPathId !== undefined
+ actualInboundPathId !== undefined && actualInboundPathId !== null
? actualInboundPathId
: inboundPathsChoices.length === 1
? inboundPathsChoices[0].value
: undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx`
around lines 81 - 93, The code currently checks actualOutboundPathId and
actualInboundPathId with !== undefined which lets null bypass the single-choice
fallback; change the checks to treat null as unset—either use a
nullish-coalescing expression (e.g., use schedulePeriod.outbound_path_id ??
(outboundPathsChoices.length === 1 ? outboundPathsChoices[0].value : '')) and
similarly for inbound (schedulePeriod.inbound_path_id ??
(inboundPathsChoices.length === 1 ? inboundPathsChoices[0].value : undefined)),
or replace the !== undefined checks with != null for both actualOutboundPathId
and actualInboundPathId so the single-choice fallback on outboundPathsChoices
and inboundPathsChoices triggers when values are null.
| display: 'flex', | ||
| flexDirection: 'row', | ||
| flexShrink: 0, | ||
| borderRight: '1px solid rgba(255,255,255,0.2)', | ||
| alignItems: 'flex-start', | ||
| minWidth: '20rem' | ||
| width: 'fit-content' | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move these new layout/width styles to SCSS classes.
These inline styles conflict with the frontend styling rule for this path; please shift them to SCSS and keep TSX class-driven.
As per coding guidelines, "React components should use Redux for state management, i18next for translations, and SCSS for styling".
Also applies to: 197-197, 207-207, 255-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/transition-frontend/src/components/forms/schedules/TransitSchedulePeriod.tsx`
around lines 181 - 187, The inline layout styles in the TransitSchedulePeriod
component (the JSX element with style={{ display:'flex', flexDirection:'row',
flexShrink:0, borderRight:'1px solid rgba(255,255,255,0.2)',
alignItems:'flex-start', width:'fit-content' }}) should be moved into SCSS:
create descriptive class(es) (e.g., .transit-schedule-period-row and variants
used in the same component) in the component's SCSS module, implement the
equivalent CSS (flex, flex-direction, flex-shrink, border-right, align-items,
width handling), replace the inline style props on the corresponding JSX
elements with className references, and apply the same refactor for the other
inline style occurrences in this file (the other similar style usages flagged in
the review). Ensure class names are unique to TransitSchedulePeriod to avoid
collisions.
There was a problem hiding this comment.
Est-ce qu'on voudrait mettre ça dans une classe scss? En aurait-on par hasard qui font déjà qqc de semblable?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for _flex-container-row and similar flex utility classes in SCSS files
rg '_flex-container-row' --type=scss -n -C3
echo "---"
# Also search more broadly for flex-container classes
rg 'flex-container' --type=scss -n -C3
echo "---"
# Check if there's a SCSS file for the TransitSchedulePeriod component
fd 'TransitSchedulePeriod' --extension=scss
echo "---"
# Look for existing SCSS flex-row patterns
rg 'flex-direction.*row|flexDirection.*row' --type=scss -n -C2
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Search for _flex-container-row in SCSS files using glob
rg '_flex-container-row' -g '*.scss' -n -C3
echo "---"
# Also search more broadly for flex-container classes
rg 'flex-container' -g '*.scss' -n -C3
echo "---"
# Check if there's a SCSS file for the TransitSchedulePeriod component
fd 'TransitSchedulePeriod' -e scss
echo "---"
# Look for fit-content or flex-shrink in scss
rg 'fit-content|flex-shrink' -g '*.scss' -n -C2
Length of output: 2713
🏁 Script executed:
#!/bin/bash
# Check full definition of _flex-container-row
rg '_flex-container-row' -g '*.scss' -n -A5 -B2
echo "---"
# Look for schedule-related SCSS files
fd -e scss | xargs grep -l -i 'schedule\|period' 2>/dev/null
echo "---"
# Check what SCSS files exist near the TransitSchedulePeriod component
fd -e scss --search-path packages/transition-frontend/src
Length of output: 720
@tahini — _flex-container-row does exist in packages/chaire-lib-frontend/src/styles/_base.scss and covers display: flex; flex-direction: row, but it also sets overflow: auto — which is precisely what was causing the nested horizontal scroll this PR is fixing! That's why the class had to be dropped.
The delta that the inline styles add on top of what _flex-container-row would provide:
align-items: flex-start(vs.stretchin the class)flex-shrink: 0border-right: 1px solid rgba(255,255,255,0.2)width: fit-content
There is no dedicated SCSS file for this component (only transition.scss and _jobTables.scss exist under packages/transition-frontend/src/styles/). Moving these to SCSS would require either a new class in transition.scss or a new SCSS module for the schedule forms. That's doable but adds scope to the PR.
A lightweight middle ground would be to keep _flex-container-row with an explicit overflow: visible override class, but given that _flex-container-row + overrides would still end up as component-specific CSS, a dedicated class in transition.scss (e.g. .tr__schedule-period-column) is probably the cleanest path if we want to follow the SCSS guideline strictly.
🧠 Learnings used
Learnt from: CR
Repo: chairemobilite/transition PR: 0
File: .cursor/rules/project-rule.mdc:0-0
Timestamp: 2026-02-03T20:40:36.982Z
Learning: Applies to packages/transition-frontend/src/components/**/*.tsx : React components should use Redux for state management, i18next for translations, and SCSS for styling
Learnt from: greenscientist
Repo: chairemobilite/transition PR: 1588
File: packages/transition-backend/src/services/transitRouting/TrRoutingBatchManager.ts:11-23
Timestamp: 2025-11-18T19:37:07.564Z
Learning: In the chairemobilite/transition repository TypeScript code, greenscientist prefers that reviews highlight when interfaces could be replaced with type aliases for stylistic consistency, especially when the structure is just a data shape.
Learnt from: kaligrafy
Repo: chairemobilite/transition PR: 1592
File: packages/transition-frontend/src/components/map/CircleSpinnerExtension.tsx:37-44
Timestamp: 2026-01-21T14:59:52.530Z
Learning: In TSX files under packages/transition-frontend/src that currently rely on deck.gl's this.context.animationProps.time for animation timing, prefer window.performance.now() (or performance.now()) to measure elapsed time. Replace time-based calculations tied to deck.gl with direct high-resolution timestamps to improve accuracy and decouple from the rendering library. Ensure animations use delta = now - lastTime, and update lastTime accordingly for smooth, frame-rate-independent timing.
iTzxMigz
force-pushed
the
fix-schedule-period-ui
branch
from
617f7e4 to
d3f38d4
Compare
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
@kaligrafy, pourrais-tu reviewer, pour les styles, c'est plus dans ton domaine.
| display: 'flex', | ||
| flexDirection: 'row', | ||
| flexShrink: 0, | ||
| borderRight: '1px solid rgba(255,255,255,0.2)', | ||
| alignItems: 'flex-start', | ||
| minWidth: '20rem' | ||
| width: 'fit-content' | ||
| }} |
There was a problem hiding this comment.
Est-ce qu'on voudrait mettre ça dans une classe scss? En aurait-on par hasard qui font déjà qqc de semblable?
- Remove nested horizontal scroll by dropping _flex-container-row class from the period (kept on the parent list only), using inline flex styles instead. - Constrain period width with maxWidth on the form section and selects so long path names no longer stretch each column. - Allow clearing outbound/inbound path when only one choice is available (distinguish 'never set' vs 'user cleared'). - Hide the Generate button when no outbound path is selected, since the backend rejects schedule generation without one. - Add optional style prop to InputSelect for per-usage width constraints.
iTzxMigz
force-pushed
the
fix-schedule-period-ui
branch
from
d3f38d4 to
96e2769
Compare
3 participants
We can see a preview here the main goal is to remove a scroll in a scroll horizontally and make generate trip without an outbound selection unavailable since it is not allowed:
Summary by CodeRabbit
Bug Fixes
Improvements