Make showFileHeaders flag functional with diffFileHeaders(_:) modifier#7
Open
paololeonardi wants to merge 2 commits intotornikegomareli:mainfrom
Open
Make showFileHeaders flag functional with diffFileHeaders(_:) modifier#7paololeonardi wants to merge 2 commits intotornikegomareli:mainfrom
paololeonardi wants to merge 2 commits intotornikegomareli:mainfrom
Conversation
📝 WalkthroughWalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
GitDiffExample/GitDiffExample/CustomizationPlayground.swift (1)
242-251: Avoid hardcoded reset values for new config fields.Using
DiffConfiguration.defaulthere prevents drift if defaults evolve.♻️ Suggested reset update
private func resetToDefaults() { withAnimation { - showLineNumbers = true - showFileHeaders = true - fontSize = 13 - lineSpacing = .compact - wordWrap = true - fontWeight = .regular - selectedTheme = .light + let defaults = DiffConfiguration.default + showLineNumbers = defaults.showLineNumbers + showFileHeaders = defaults.showFileHeaders + fontSize = defaults.fontSize + lineSpacing = defaults.lineSpacing + wordWrap = defaults.wordWrap + fontWeight = defaults.fontWeight + selectedTheme = defaults.theme } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@GitDiffExample/GitDiffExample/CustomizationPlayground.swift` around lines 242 - 251, The resetToDefaults() function currently hardcodes each config value; instead fetch the canonical defaults from DiffConfiguration.default and apply them so new fields won't be missed—e.g., inside withAnimation let defaults = DiffConfiguration.default and then assign showLineNumbers = defaults.showLineNumbers, showFileHeaders = defaults.showFileHeaders, fontSize = defaults.fontSize, lineSpacing = defaults.lineSpacing, wordWrap = defaults.wordWrap, fontWeight = defaults.fontWeight, selectedTheme = defaults.selectedTheme (or replace the whole config object if this type supports assignment), ensuring any newly added DiffConfiguration fields are automatically picked up.Sources/gitdiff/DiffEnvironment.swift (1)
64-82: Use the builder to avoid config-field drift indiffFileHeaders(_:).This works, but rebuilding the entire config here duplicates state-copy logic and can silently miss new fields later. Reuse the builder for single-point maintenance.
♻️ Suggested simplification
func diffFileHeaders(_ show: Bool) -> some View { transformEnvironment(\.diffConfiguration) { config in - config = DiffConfiguration( - theme: config.theme, - showLineNumbers: config.showLineNumbers, - showFileHeaders: show, - fontFamily: config.fontFamily, - fontSize: config.fontSize, - fontWeight: config.fontWeight, - lineHeight: config.lineHeight, - lineSpacing: config.lineSpacing, - wordWrap: config.wordWrap, - contentPadding: config.contentPadding - ) + config = config.withFileHeaders(show) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/gitdiff/DiffEnvironment.swift` around lines 64 - 82, The diffFileHeaders(_:) function is rebuilding a full DiffConfiguration which risks drifting when fields are added; instead use the existing DiffConfiguration's builder/with-style API to modify only showFileHeaders. In the transformEnvironment(\.diffConfiguration) closure, call the configuration's builder/with method (e.g., config.with(showFileHeaders: show) or config.builder().setShowFileHeaders(show).build()) to produce the new config so all other fields are preserved via the config's own update helper rather than reconstructing DiffConfiguration manually.GitDiffExample/GitDiffExample/ExamplesView.swift (1)
242-249: Consider a responsive layout for the two toggles + action button row.With larger text sizes, this single
HStackcan get crowded. AVStack/ViewThatFitssplit would improve resilience.♿️ Possible responsive layout
- HStack { - Toggle("Line Numbers", isOn: $showLineNumbers) - Toggle("File Headers", isOn: $showFileHeaders) - Spacer() - Button(action: shareExample) { - Image(systemName: "square.and.arrow.up") - } - } + ViewThatFits(in: .horizontal) { + HStack { + Toggle("Line Numbers", isOn: $showLineNumbers) + Toggle("File Headers", isOn: $showFileHeaders) + Spacer() + Button(action: shareExample) { + Image(systemName: "square.and.arrow.up") + } + } + VStack(alignment: .leading, spacing: 8) { + Toggle("Line Numbers", isOn: $showLineNumbers) + Toggle("File Headers", isOn: $showFileHeaders) + HStack { + Spacer() + Button(action: shareExample) { + Image(systemName: "square.and.arrow.up") + } + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@GitDiffExample/GitDiffExample/ExamplesView.swift` around lines 242 - 249, The current HStack containing the two Toggles (showLineNumbers, showFileHeaders) and the shareExample Button can overflow at large accessibility text sizes; refactor this row to use a responsive container such as ViewThatFits or a conditional VStack/HStack so the toggles stack vertically when space is constrained. Locate the HStack block that wraps Toggle("Line Numbers", isOn: $showLineNumbers), Toggle("File Headers", isOn: $showFileHeaders) and Button(action: shareExample) and replace it with a ViewThatFits (or use GeometryReader/Environment(\.sizeCategory) to switch to a VStack) so the layout automatically switches to a vertical arrangement for larger text sizes while preserving the same controls and actions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@GitDiffExample/GitDiffExample/CustomizationPlayground.swift`:
- Around line 242-251: The resetToDefaults() function currently hardcodes each
config value; instead fetch the canonical defaults from
DiffConfiguration.default and apply them so new fields won't be missed—e.g.,
inside withAnimation let defaults = DiffConfiguration.default and then assign
showLineNumbers = defaults.showLineNumbers, showFileHeaders =
defaults.showFileHeaders, fontSize = defaults.fontSize, lineSpacing =
defaults.lineSpacing, wordWrap = defaults.wordWrap, fontWeight =
defaults.fontWeight, selectedTheme = defaults.selectedTheme (or replace the
whole config object if this type supports assignment), ensuring any newly added
DiffConfiguration fields are automatically picked up.
In `@GitDiffExample/GitDiffExample/ExamplesView.swift`:
- Around line 242-249: The current HStack containing the two Toggles
(showLineNumbers, showFileHeaders) and the shareExample Button can overflow at
large accessibility text sizes; refactor this row to use a responsive container
such as ViewThatFits or a conditional VStack/HStack so the toggles stack
vertically when space is constrained. Locate the HStack block that wraps
Toggle("Line Numbers", isOn: $showLineNumbers), Toggle("File Headers", isOn:
$showFileHeaders) and Button(action: shareExample) and replace it with a
ViewThatFits (or use GeometryReader/Environment(\.sizeCategory) to switch to a
VStack) so the layout automatically switches to a vertical arrangement for
larger text sizes while preserving the same controls and actions.
In `@Sources/gitdiff/DiffEnvironment.swift`:
- Around line 64-82: The diffFileHeaders(_:) function is rebuilding a full
DiffConfiguration which risks drifting when fields are added; instead use the
existing DiffConfiguration's builder/with-style API to modify only
showFileHeaders. In the transformEnvironment(\.diffConfiguration) closure, call
the configuration's builder/with method (e.g., config.with(showFileHeaders:
show) or config.builder().setShowFileHeaders(show).build()) to produce the new
config so all other fields are preserved via the config's own update helper
rather than reconstructing DiffConfiguration manually.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24d61140-d091-426a-a1cf-de8ec657ca72
📒 Files selected for processing (6)
GitDiffExample/GitDiffExample/CustomizationPlayground.swiftGitDiffExample/GitDiffExample/ExamplesView.swiftREADME.mdSources/gitdiff/DiffConfigurationBuilder.swiftSources/gitdiff/DiffEnvironment.swiftSources/gitdiff/Views/DiffFileView.swift
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
1 participant
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.
@tornikegomareli great project, thanks for open sourcing it!
Summary
The
showFileHeadersflag inDiffConfigurationexisted but was never consulted — file headers were always rendered. This PR makes it work, exposes it as a first-class view modifier, and documents it.Changes
DiffFileView: conditionally render the file header based onconfiguration.showFileHeadersDiffEnvironment: add publicdiffFileHeaders(_:)view modifier, matching the ergonomics ofdiffLineNumbers(_:)DiffConfigurationBuilder: addwithFileHeaders(_:)builder method for consistency with the rest of the builder APIREADME: add.diffFileHeaders(_:)to the modifiers list andshowFileHeadersto the configuration object exampleCustomizationPlaygroundandExamplesViewTesting
Verified manually via the example app — toggling the control in both the Customization Playground and Examples view shows/hides file headers as expected.
Summary by CodeRabbit
New Features
.diffFileHeaders()view modifier for controlling header visibility in the configuration chainDocumentation